-
Notifications
You must be signed in to change notification settings - Fork 442
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
Dependency cleanup and Python 3.12 support #2953
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: pypi/[email protected]), pypi/[email protected]), pypi/[email protected]), pypi/[email protected]), pypi/[email protected]) |
Btw, @schustmi, this PR is still early in development. I was just using it to test things on the CI. Will update you as things move forward. |
Classification template updates in |
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.
Overall looks good, but I had some nits and questions here and there.
tests/integration/integrations/deepchecks/materializers/test_deepchecks_result_materializer.py
Show resolved
Hide resolved
# TODO: Remove and add the extra back in | ||
uv pip install $PIP_ARGS git+https://github.com/zenml-io/mlstacks.git@feature/upgrade-python-3.12 |
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.
Please remember to remove this
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.
Thank you for the reminder. I had to add one more of this to the small-checks
workflow in the CI, unfortunately. We have to keep them until we can release a new version of mlstacks
. I will keep this thread unresolved as a reminder to myself.
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.
@stefannica Just so you know, this is going to be a bit tricky as I can not remove these lines with this PR.
Right now, the changes that I made are not compatible with the latest release of mlstacks
(0.9.0). If I remove them now and merge to develop
, all the subsequent CI runs on other PRs branching off of develop
will fail.
We can potentially release a new version of mlstacks
as a solution but if we do that mlstacks
will be broken until we release a new version of zenml
. (There won't be any zenml
versions which can work with this new mlstacks
release.)
So, my proposal would be something like this:
- I merge this PR while keeping
install-zenml-dev
andsmall-checks
pointing to my upgrade branch on themlstacks
repo. - I create a follow-up PR which actually reverts them back.
- Just before we release a new version of
zenml
, we do themlstacks
release, merge the PR from point 2 and proceed with the release.
This needs to be communicated to the product team as well, I will take over that.
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 agree with the concerns raised by @stefannica. On top, I added a few minor comments.
Co-authored-by: Andrei Vishniakov <[email protected]>
from zenml.zen_server.secure_headers import ( | ||
initialize_secure_headers, | ||
secure_headers, | ||
) |
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.
Nice move 👍🏼
LLM Finetuning template updates in |
E2E template updates in |
NLP template updates in |
Classification template updates in |
Describe changes
Major changes
click-params
,httplib2
,pyparsing
as main dependencies.secure
to theserver
extranumpy
andpandas
to implementations to their own integrations.maison
to solve a problem with theyamlfix
installation.pydantic
to2.8.*
zenml
to Python 3.12. The new version is also added to all relevant CI tests.Minor changes
PydanticMaterializer
instead of thePandasMaterializer
.pandas
version in thedeepchecks
integration to<2.1.0
.PandasMaterializer
and theNumpyMaterializer
.mlflow
integration slightly reworked. The extra deployment optionmlserver
andmlserver-mlflow
only installed <3.12 now.skimage
. Previously, we were able to access this package (for instance inmlflow
andseldon
example tests) becausedeepchecks
was installed in the same environment anddeepchecks
required it (because we installed all the integration together). However, on 3.12,deepchecks
integration is still not supported -> it is not installed -> there is noskimage
->mlflow
andseldon
example tests fail. The package itself is around 14 MB so I added it to thesklearn
integration.Related PRs
Remaining TODOs
Investigate if we should implement a migration script to change the materializer entries to newer paths.Investigate Windows testing on 3.12.Add warnings/errors for integrations that are not yet supported in 3.12..Ignore the integration tests on 3.12 for integrations that are not yet supported in 3.12..Revert the changes in the CI.Create new versions of all templates.Investigatehttplib2
andpyparsing
issue.Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes