Skip to content
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

Skip version check for incompatible version strings in bootstrapper.py #3106

Merged
merged 2 commits into from
Feb 14, 2023
Merged

Conversation

cjackal
Copy link
Contributor

@cjackal cjackal commented Feb 10, 2023

Signed-off-by: cjackal juhn3707@gmail.com

What changes were proposed in this pull request?

Current runtime dependency version check routine for elyra pipeline is less robust on source packages not installed via pypi.
E.g. one may struggle to run elyra pipeline on clean-installed miniconda base environment, which pip freeze to file:// protocol:

in 24b445e

juhn3@vscode-0:~$ miniconda3/bin/python git/elyra/elyra/kfp/bootstrapper.py -e aaa -b aaa -d aaa -t aaa -f aaa -n aaa
[D 02:55:13.869] Parsing Arguments.....
[I 02:55:13.870] 'aaa':'aaa' - starting operation 
[I 02:55:13.870] 'aaa':'aaa' - Installing packages 
[I 02:55:13.870] Package not found. Installing ipykernel package with version 6.13.0...
[I 02:55:13.870] Package not found. Installing ipython package with version 8.3.0...
[I 02:55:13.870] Package not found. Installing ipython-genutils package with version 0.2.0...
[I 02:55:13.870] Package not found. Installing jinja2 package with version 3.0.3...
[I 02:55:13.870] Package not found. Installing jupyter-client package with version 7.3.1...
[I 02:55:13.870] Package not found. Installing jupyter-core package with version 4.11.2...
[I 02:55:13.870] Package not found. Installing MarkupSafe package with version 2.1.1...
[I 02:55:13.870] Package not found. Installing minio package with version 7.1.8...
[I 02:55:13.870] Package not found. Installing nbclient package with version 0.6.3...
[I 02:55:13.870] Package not found. Installing nbconvert package with version 6.5.1...
[I 02:55:13.871] Package not found. Installing nbformat package with version 5.4.0...
[I 02:55:13.871] Package not found. Installing papermill package with version 2.3.4...
[I 02:55:13.871] Package not found. Installing pyzmq package with version 24.0.1...
[I 02:55:13.871] Package not found. Installing prompt-toolkit package with version 3.0.29...
Traceback (most recent call last):
  File "/home/juhn3/git/elyra/elyra/kfp/bootstrapper.py", line 742, in <module>
    main()
  File "/home/juhn3/git/elyra/elyra/kfp/bootstrapper.py", line 725, in main
    OpUtil.package_install(user_volume_path=input_params.get("user-volume-path"))
  File "/home/juhn3/git/elyra/elyra/kfp/bootstrapper.py", line 563, in package_install
    elif version.parse(ver) > version.parse(current_packages[package]):
  File "/home/juhn3/miniconda3/lib/python3.10/site-packages/packaging/version.py", line 52, in parse
    return Version(version)
  File "/home/juhn3/miniconda3/lib/python3.10/site-packages/packaging/version.py", line 197, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: 'file:///opt/conda/conda-bld/requests_1657734628632/work'

This PR suggests to skip version check for every source packages without proper version information by screening packaging.version.InvalidVersion exception.

How was this pull request tested?

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: cjackal <juhn3707@gmail.com>
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes look good to me, but I'd like Alan to take a look and have requested his review.

@ptitzler ptitzler added kind:bug Something isn't working component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines labels Feb 13, 2023
@ptitzler
Copy link
Member

Would you mind applying the same fix also to the Apache Airflow https://github.com/cjackal/elyra/blob/main/elyra/airflow/bootstrapper.py#L346 since the issue isn't runtime specific?

@cjackal
Copy link
Contributor Author

cjackal commented Feb 13, 2023

@ptitzler While the same fix can be done, it looks like airflow version relies on depreciated (in fact, removed since packaging==22.0) packaging.version.LegacyVersion and I assumed up-to-date packaging version. IIUC, the intended behavior here (for both bootstrapper) is as non-disruptive as possible(skip, I mean) for PEP-440 incompliant version strings, am I right? Then I'll give a better patch applicable to both bootstrappers.

EDIT. In short, requirements-elyra.txt will only contain PEP-440 compliant versions, right?

@ptitzler
Copy link
Member

@ptitzler While the same fix can be done, it looks like airflow version relies on depreciated (in fact, removed since packaging==22.0) packaging.version.LegacyVersion and I assumed up-to-date packaging version.

Sorry I missed that. And yes, you are right. The majority of Elyra deployments we are aware of are utilizing Kubeflow Pipelines as runtime and therefore, over time, we've invested more in that code base. As a result there are now some differences between the two files, with the KFP version being the up-to-date one.

EDIT. In short, requirements-elyra.txt will only contain PEP-440 compliant versions, right?

Yes! At least the official version of the file on GitHub will.

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Also update version check to work the same on `packaging<22.0`

Signed-off-by: cjackal <juhn3707@gmail.com>
@cjackal
Copy link
Contributor Author

cjackal commented Feb 14, 2023

Thanks for clarification @ptitzler !

A comment on the changes in this PR:

There was a breaking API change since packaging==22.0:
packaging.version.parse(ver) now raise packaging.version.InvalidVersion error for invalid version strings.
(return packaging.version.LegacyVersion(version) with depreciation warning before)

As version.parse just passes its argument to version.Version(and falls back to version.LegacyVersion in older packaging) and the behavior (raise InvalidVersion error) for Version is stable (since like ~5 years?), replacing version.parse with version.Version is backward compatible and make screening with InvalidVersion work on wider versions of packaging.

Copy link
Member

@akchinSTC akchinSTC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjackal - LTGM and thanks for your first contribution and merge!

@akchinSTC akchinSTC merged commit cd7e295 into elyra-ai:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines kind:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants