Introduce formal principle documentation to support contrib changes #3768
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3768 +/- ##
==========================================
- Coverage 89.23% 89.23% -0.01%
==========================================
Files 907 907
Lines 104817 104817
==========================================
- Hits 93536 93532 -4
- Misses 11281 11285 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
I have a bunch of minor edit suggestions but overall I think this looks great!
michaelbynum
left a comment
michaelbynum
left a comment
There was a problem hiding this comment.
I'm only about halfway done, but here are a couple comments/questions.
| These commands register the cloned code with the active python environment | ||
| (``pyomodev``). This way, your changes to the source code for ``pyomo`` are | ||
| (``pyomodev``) and installs all possible optional dependencies. | ||
| This way, your changes to the source code for ``pyomo`` are |
There was a problem hiding this comment.
"changes to the source code for pyomo are automatically used - this is from the -e, but, from the way this is written, it sounds like this is because optional dependencies have been installed.
| * The package includes a README or module-level documentation that | ||
| clearly describes its purpose and usage. |
There was a problem hiding this comment.
Is this necessary if docs are added to the online docs?
There was a problem hiding this comment.
I would prefer we request that documentation go into the online docs and actually discourage READMEs.
michaelbynum
left a comment
michaelbynum
left a comment
There was a problem hiding this comment.
I have some minor comments, but I think this looks great overall.
| change until the next major Pyomo release, which will coincide with a | ||
| new edition of the book. |
There was a problem hiding this comment.
While this is what we currently do, it is a strong statement/commitment. I'm fine with this, but I wanted to flag it for discussion.
There was a problem hiding this comment.
Yeah, I might be for hedging and just saying next major Pyomo release and failing to mention the promise of books?
There was a problem hiding this comment.
We talked about this in the dev call today (loosely). This is our "current" approach but nothing says we can't change it in the future, especially after the next book.
There was a problem hiding this comment.
We can always hedge our promise with something like will hopefully coincide
|
|
||
| Functionality that is part of the Pyomo source tree but not explicitly | ||
| included in the book is also expected to be stable if it resides outside | ||
| the ``contrib`` (or future ``addons`` / ``devel``) directories. This is |
There was a problem hiding this comment.
should this just say "devel" instead of "addons / devel"? I think the contribution guide stated that addons was supposed to be stable as well.
There was a problem hiding this comment.
I'm specifically looking under "Namespaces for Contributed and Experimental Code" in contribution_guide.rst.
| Avoid ``from pyomo.environ import *`` or alternative aliases. | ||
| Additionally, avoid all uses of ``import *``. |
There was a problem hiding this comment.
avoid import * is listed twice.
There was a problem hiding this comment.
That's how much Miranda cares!! :P
| # Pyomo Addons | ||
|
|
||
| The `pyomo.addons` directory contains **mostly stable extensions** to Pyomo | ||
| that are **not part of the core library*. These packages extend Pyomo's |
| - Represent functionality that is **useful to the wider Pyomo community** but | ||
| not required for core operation. |
There was a problem hiding this comment.
I don't think this bullet adds any value, and I think it could be misconstrued.
| Examples include: | ||
| - `gdpopt` | ||
| - `incidence_analysis` | ||
| - `latex_printer` | ||
| - `trustregion` | ||
| - `viewer` |
There was a problem hiding this comment.
can't people just look to see what is in the addons directory? Why list it here?
There was a problem hiding this comment.
I have an answer to this one! Because as of this PR, there will be nothing in the addons directory, so I wanted to list some actual examples while we go through the process of actually moving everything.
| - (PREFERABLE) A **reference page** in the Pyomo online documentation. | ||
| - (MINIMUM) A **README.md** file summarizing: | ||
| - Functionality | ||
| - Example usage or link to docs |
There was a problem hiding this comment.
I think "or link to docs" answers one of my earlier questions.
| - Declared in `setup.py` under the appropriate category. | ||
| - Publicly available on PyPI and/or Anaconda and reasonably maintained. | ||
|
|
||
| ### Backward Compatibility |
There was a problem hiding this comment.
Is this section a duplicate of "Stability and Maintenance" above?
| Examples include: | ||
| - `doe` | ||
| - `piecewise` | ||
| - `solver` | ||
| - `sensitivity_toolbox` |
There was a problem hiding this comment.
Again, I don't think this list is needed.
| *.egg-info/ | ||
|
|
||
| # Documentation builds | ||
| doc/OnlineDocs/api |
There was a problem hiding this comment.
This is auto-generated stuff?
There was a problem hiding this comment.
Yup - when you run make -C doc/OnlineDocs html, this is all the automatic api docs that get generated. Which are great, we love them, but I have almost accidentally committed them way too many times.
emma58
left a comment
emma58
left a comment
There was a problem hiding this comment.
This looks nice--I put a lot of ideas in the comments.
| The core development team tries to review PRs in a timely | ||
| manner, but we make no guarantees on review timeframes. In addition, PRs | ||
| might not be reviewed in the order in which they are opened. |
There was a problem hiding this comment.
Perhaps it's worth mentioning that we much prefer multiple small PRs rather than single enormous ones? (And that this effects our review time...)
| contributed packages are treated as optional packages, which are not | ||
| maintained by the Pyomo developer team. Thus, it is the responsibility |
There was a problem hiding this comment.
Maybe "not necessarily maintained by the Pyomo developer team" or "not promised to be maintained by the Pyomo development team"? We play here too, and we do occasionally adopt things...
| * Optional dependencies are properly declared in ``setup.py`` | ||
| under the appropriate ``[optional]`` section. | ||
| * The contribution passes all standard style and formatting checks. | ||
|
|
There was a problem hiding this comment.
Do we want to add anything here regarding imports? We tend to put contrib things in the pyomo.environ init file. Which is a choice. But it might be worth documenting it if we want people to do it because I think that's pretty opaque to new contributors.
There was a problem hiding this comment.
Oh, just kidding, you address this below. Perhaps it's worth including where to go to get the imports right though.
| Optional Dependencies | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Extensions to Pyomo, and many of the contributions in ``pyomo.contrib``, |
There was a problem hiding this comment.
Should pyomo.contrib be updated to be pyomo.addons and pyomo.devel here?
|
|
||
| The Pyomo development team follows a set of development principles. | ||
| In order to promote overall transparency, this page is intended to document | ||
| those principles, to the best of our ability, for users and potential |
There was a problem hiding this comment.
No comma after principles, I think.
| Avoid ``from pyomo.environ import *`` or alternative aliases. | ||
| Additionally, avoid all uses of ``import *``. |
There was a problem hiding this comment.
That's how much Miranda cares!! :P
| * **Print statements:** Avoid printing directly to ``stdout``. Pyomo is a | ||
| library, not an application, and copious output can interfere with downstream | ||
| tools and workflows. Use the appropriate logger instead. Print | ||
| information only when the user has enabled or requested it. |
There was a problem hiding this comment.
Maybe we should add one about logging below this: We have settled on usually naming the logger using __name__, and I think we like that?
| * **Pull Request naming:** Pull Request titles are added to the CHANGELOG | ||
| and the release notes. The Pyomo development team reserves the right to | ||
| alter titles as appropriate to ensure they fit the look and feel of | ||
| other titles in the CHANGELOG. |
There was a problem hiding this comment.
@blnicho, didn't you have more specific guidance for this at one point too? Character limit and starting with "package:" or something like that that we fail to do most of the time?
| # This software is distributed under the 3-clause BSD License. | ||
| # ___________________________________________________________________________ | ||
| Update the year range as appropriate when modifying files. |
There was a problem hiding this comment.
A few more in-the-weeds ideas we could add to this list (or defer for a more in-the-weeds thing later--I'm not super attached to any of these, just brainstorming):
- Loud failure is always better than silent failure: Write code to guard against cases you don't expect, even if you just raise
NotImplementedErrorsto start rather than handling them. The worst Pyomo bugs are the ones that silently do wrong or unexpected math. - Along this vein, if the behavior a user could reasonably expect is ambiguous, document assumptions well and fail loudly when they are violated.
- Some Pyomo components are
ActiveComponentsand others are not (perhaps most importantly, Vars are not)... We define models to be the set of active components accessible from the root Block. This means many things, but in common gotchas, thatm.component_data_objects(Var, ...)is almost always a bad idea: Usually you wantget_vars_from_componentswithConstraintor(Constraint, Objective)as thectypeargument. - Writers and their ilk should always complain about active Pyomo components they do not recognize. (This is because Pyomo supports extended modeling environments such as DAE and GDP so it is easy to accidentally send unexpected structures to solvers expecting algebraic model formulations). There is a utility for this in
pyomo.repn.utilcalledcategorize_valid_components. - We prefer not to use component names (or strings in general) to keep track of them in writers, algorithms, etc.
pyomo.common.collectionsincludes data structures where unhashable components (e.g., Vars) can be used as keys. - We prefer PEP8 naming conventions (descriptive names, snake case for functions, pascal case for classes, etc.) and also prefer functions be verb phrases and classes be noun-ish.
- We have a deadly allergy to repeated (and especially copy-pasted) code--we're always happy to talk about design if something seems to be headed that way
There was a problem hiding this comment.
Edit to the third bullet based on dev call discussion: If you want to gather all the variables on the model, it is recommended to use get_vars_from_components with (Constraint, Objective) as the ctype argument. This is as opposed to m.component_data_objects(Var, ...), which will get all the Vars declared on the model hierarchy, which is highly unlikely to be what is desired, as it has no mathematical meaning.
There was a problem hiding this comment.
I strongly agree with all of these.
| @TransformationFactory.register( | ||
| 'contrib.example.xfrm', | ||
| doc="An example of a transformation in a pyomo.contrib package", | ||
| 'devel.example.xfrm', doc="An example of a transformation in a pyomo.devel package" | ||
| ) | ||
| class Xfrm_PyomoTransformation(Transformation): | ||
| def __init__(self): |
There was a problem hiding this comment.
Perhaps while we're making this change, we should adjust naming here? I'd suggest devel.example.transform with a class name of just ExamplePyomoTransformation or something. I also think it would be good to not have a plugins directory in example and just flatten it out like we wish we had so many other places...
| framework with Pyomo-specific capabilities, including automatic plugin | ||
| discovery, solver availability checks, and improved test filtering | ||
| controls. Using the provided interface ensures that all tests run |
There was a problem hiding this comment.
This is not technically true (the availability / plugin / filtering from conftest.py has nothing to do with Pyomo's custom unittest module.
| framework with Pyomo-specific capabilities, including automatic plugin | |
| discovery, solver availability checks, and improved test filtering | |
| controls. Using the provided interface ensures that all tests run | |
| framework with Pyomo-specific capabilities, including support for test | |
| timeouts and Pyomo-specific assertions for comparing expressions and | |
| nested containers with numerical tolerance. | |
| Using the provided interface ensures that all tests run |
| manner but we make no guarantees on review timeframes. In addition, PRs | ||
| might not be reviewed in the order they are opened in. | ||
| * **Core and Addons:** Code rigor, standards compliance, test coverage above | ||
| a threshold, and avoidance of unintended side effects (e.g., regressions) |
There was a problem hiding this comment.
| a threshold, and avoidance of unintended side effects (e.g., regressions) | |
| a threshold, and avoidance of unintended side effects (e.g., regressions or backwards incompatibilities) |
| Namespaces for Contributed and Experimental Code | ||
| ++++++++++++++++++++++++++++++++++++++++++++++++ | ||
|
|
||
| Pyomo has a long history of supporting community-developed extensions. |
There was a problem hiding this comment.
| Pyomo has a long history of supporting community-developed extensions. | |
| Pyomo has a long history of encouraging and distributing community-developed extensions. |
|
|
||
| As a result, Pyomo is transitioning to a more structured contribution | ||
| model with two clear namespaces: | ||
|
|
There was a problem hiding this comment.
| * ``pyomo`` core - These modules form the foundation of the Pyomo environment, | |
| including the base expression systems, modeling components, model compilers, | |
| and solver interfaces. The core development team has committed to | |
| maintaining these capabilities, adhering to the strictest policies for | |
| testing and backwards compatibility. |
|
|
||
| Contrib Packages within Pyomo | ||
| +++++++++++++++++++++++++++++ | ||
| When submitting a new contributed package (under either ``addons`` or |
There was a problem hiding this comment.
| When submitting a new contributed package (under either ``addons`` or | |
| When submitting a new package (under either ``addons`` or |
| * **Circular imports:** Avoid importing from ``pyomo.core`` into any module | ||
| in ``pyomo.common`` due to the potential for circular imports. |
There was a problem hiding this comment.
| * **Circular imports:** Avoid importing from ``pyomo.core`` into any module | |
| in ``pyomo.common`` due to the potential for circular imports. | |
| * **Circular imports:** Make every effort to avoid circular imports. When | |
| circular imports are absolutely necessary, leverage :py:func`attempt_import()` | |
| to explicitly break the cycle. To help with this, some module namespaces | |
| have additional requirements: | |
| * ``pyomo.common``: modules within :py:mod:`pyomo.common` *must* not | |
| import *any* Pyomo modules outside of :py:mod:`pyomo.common` | |
| * ``pyomo.core.expr``: modules within :py:mod:`pyomo.core.expr` should not | |
| import modules outside of :py:mod:`pyomo.common` | |
| or :py:mod:`pyomo.core.expr`. | |
| * ``pyomo.core.base``: modules within :py:mod:`pyomo.core.base` should not | |
| import modules outside of :py:mod:`pyomo.common`, | |
| :py:mod:`pyomo.core.expr`, or :py:mod:`pyomo.core.base`. |
| * **Circular imports:** Avoid importing from ``pyomo.core`` into any module | ||
| in ``pyomo.common`` due to the potential for circular imports. | ||
|
|
||
| * **Print statements:** Avoid printing directly to ``stdout``. Pyomo is a |
There was a problem hiding this comment.
| * **Print statements:** Avoid printing directly to ``stdout``. Pyomo is a | |
| * **Print statements:** Avoid printing or writing directly to ``stdout``. Pyomo is a |
| * **File headers:** Every ``.py`` file must begin with the standard Pyomo | ||
| copyright header: | ||
|
|
||
| .. code-block:: text |
There was a problem hiding this comment.
Can this be .. literalinclude::'ed from pyomo/__init__.py (that way the documentation is kept in sync with the codebase)?
| # This software is distributed under the 3-clause BSD License. | ||
| # ___________________________________________________________________________ | ||
| Update the year range as appropriate when modifying files. |
| from pyomo.contrib.example.foo import * | ||
| from pyomo.contrib.example import bar | ||
| from pyomo.devel.example.foo import a | ||
| from pyomo.devel.example import bar |
There was a problem hiding this comment.
I would like to propose removing contrib/example entirely and replacing it with a section in the Online Docs
5 participants
Fixes None but starts to address #2460
Summary/Motivation:
Stage 1 of the changes inside
pyomo.contribis documenting a lot of different development principles that make the change a lot more reasonably understandable. This PR creates the new directories with their basic READMEs as well as creates documentation about our overarching development principles, including a teaser to the upcoming changes incontribas part of their contents.Changes proposed in this PR:
addonsanddevelprinciples.rstdocumentationdoc/OnlineDocs/apiin .gitignore because I have almost accidentally committed those files WAY too many timesLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: