-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spring cleaning for dependencies, Pt. 2: The Tree-Chopping #1133
Conversation
This is waiting for #1138 |
This reverts commit 1176954.
"Explicit is better than implicit"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments that I wanted to check on, but otherwise this is looking good to me.
|
||
pip install idaes-pse==1.13 | ||
pip install idaes-pse==2.0 | ||
|
||
* To get the latest version from the GitHub main branch :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I do not see here is instructions on how to get the optional dependencies; this is one of our user facing sets of install instructions so we probably need to cover it here (or point to the README for more details and to avoid the possibility of conflicting instrucitons).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I agree that the optional dependencies instructions should be included in the user-facing installation material. I think I'll copy what's in the README to the Sphinx docs, and we can revisit later if the repetition becomes a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a How-to page on optional dependencies: https://idaes-pse--1133.org.readthedocs.build/en/1133/how_to_guides/opt_dependencies.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Remove extra indentation * Use hyphen in package distribution name * Update minimum Python version * Reorganize optional dependencies * Introduce requirements-dev.txt file * Try removing idaes-pse from requirement file * Try using IDAES/idaes-pse#1133 for dev requirements * Add dependencies for surrogates target * Add (temporarily) dmf target for convergence_base * Rename target to omlt * Remove dmf extras_require * Try removing setup.py * Remove ref to merged PR * Add test for idaes_examples.browse.find_notebook_dir() * Add gui target to dev dependencies * Add requirement for importlib_resources backport for 3.8
Resolves: #705
Motivation
pip install "."
from this branch in a fresh environment results in the following being installed:ui,dmf,grid,omlt
targets:pip install ".[ui,dmf,grid,omlt]"
Changes proposed in this PR:
extras_require
targetsCaveats
Most likely causes widespread test failuresTests are now passingWould still need to update the documentationDocumentation has been updated (I might have missed something)Decisions
extras_require
target (e.g.complete
,all
). This follows the "explicit is better than implicit" principle and requires downstream clients to pro-actively "opt in" to the sets of optional dependencies they needFor reviewers
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: