-
Notifications
You must be signed in to change notification settings - Fork 862
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
Add versioning release instructions, add boto3 version range #2829
Conversation
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!
* Ensure all dependencies have an upper cap, unless the reason is documented inline. | ||
* Example of no upper cap: `scikit-learn>=1.0` |
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.
Discussion point: an Introduction of a overly aggressive upper cap would require binary releases every time we want to change the range. This significantly increase work for DLC patching (~1 wk becomes 3-4 weeks).
Pinning pros: repeatable builds
Pinning cons: require more frequent releases as the new CVEs coming.
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.
We already have aggressive upper caps on almost every dependency. We are planning to relax some of them this release (Numpy and SciPy)
2. Suppress the warning for the user + provide justification and appropriate TODOs. | ||
3. Avoid the range change and add an inline comment in setup.py that a warning occurred and why we didn't fix. | ||
* Ensure CI passes, potentially benchmark to catch performance regressions / more complex bugs. | ||
* Note: To truly catch potential errors, you will need to use the latest supported Python version, since some packages may only support the newer Python version in their latest releases. |
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.
We really need to test this on both sides of supported ranges: i.e. numpy at some point stopped 3.7 support; so 3.7 was stuck at <1.21, while 3.8/3.9 was 1.21+
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.
Yes, technically it is more complex to comprehensively test. We can add additional steps in future PRs.
"statsmodels~=0.13.0", | ||
"gluonts~=0.12.0", |
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.
~=0.1.3.0
-> ~=0.13
?
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.
This PR is not touching the version ranges to minimize potential merge conflicts with other PRs. A follow-up PR will be sent to clean up the ranges.
"statsmodels~=0.13.0", | ||
"gluonts~=0.12.0", | ||
"torch>=1.9,<1.14", | ||
"pytorch-lightning>=1.7.4,<1.9.0", | ||
"networkx", | ||
"networkx", # version range defined in `core/_setup_utils.py` | ||
"statsforecast==1.4.0", |
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.
ditto; or if we pin, needs a comment
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.
Tracked in #2831
Job PR-2829-cec3bbb is done. |
Job PR-2829-c6de042 is done. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.