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

[VR-12958] Replace git-installed verta in requirements with a pinned version #2643

Merged
merged 7 commits into from Sep 22, 2021

Conversation

convoliution
Copy link
Contributor

@convoliution convoliution commented Sep 20, 2021

Recommend reviewing with "hide whitespace changes"

Impact and Context

This PR addresses two issues:

  1. If a user's environment has git+https://github.com/VertaAI/modeldb.git#egg=verta, Python(requirements) wouldn't recognize this and would append verta=={current_version}, resulting in a duplicate requirement and an installation error.
  2. If a user's environment has git+ssh://git@github.com/VertaAI/modeldb.git#egg=verta, this has the above issue, but also wouldn't work anyway because the model build machinery may not have the requisite ssh credentials.

The solution chosen is to deliberately replace all git installations of the verta package with verta=={current_version}.

Risks

This means that a user working locally with a non-publicly-published version of the client—when they deploy a model—will have their client version effectively "rounded down" to the nearest public version. But aside from the brief window of this past week, this has always been the case!

Testing

  • Deployed the service to dev env
    • no new service
  • Used functionality on dev env
    • deployed a model using Python(Python.read_pip_environment()) and it worked
  • Added unit test(s)
    • test_utils/test_pip_requirements.py::TestPinVertaAndCloudpickle::test_inject_requirement
  • Added integration test(s)
    • versioning/environment/test_python.py::TestVCSInstalledVerta::test_vcs_installed_verta

test_utils/test_pip_requirements.py and versioning/environment/test_python.py all pass in Jenkins in Python 2.7 and 3.7.

How to Revert

Revert this PR.

@@ -35,7 +35,6 @@ def requirements_file_with_unsupported_lines():
"-c some_constraints.txt",
"-f file://dummy",
"-i https://pypi.org/simple",
"-e git+ssh://git@github.com/VertaAI/modeldb.git@master#egg=verta&subdirectory=client/verta",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this because it's no longer an "unsupported line" (instead, it gets swapped out with "verta" before supported-ness is even checked).

Copy link
Contributor

@daniel-verta daniel-verta left a comment

Choose a reason for hiding this comment

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

Feel free to contest or discuss my request to favor a pure functions approach for methods such as inject_requirements.

Outside of the scope of this change, I'll note that we can also make our code surrounding Python requirements more robust by using the setuptools library and its models for Python requirements, e.g. Requirement Objects. I'll make a ticket to capture that improvement which we can then prioritize accordingly.

@convoliution convoliution merged commit 67622fd into master Sep 22, 2021
@convoliution convoliution deleted the ml/log-reqs-bugs branch September 22, 2021 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants