-
Notifications
You must be signed in to change notification settings - Fork 240
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
[Builder] Install mlrun with python requirements #3673
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.
very well! minor comment
mlrun/api/utils/builder.py
Outdated
|
||
# mlrun must be installed with other python requirements in the same pip command to avoid version conflicts | ||
if mlrun_version: | ||
requirements_list = [mlrun_version] + requirements_list |
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.
requirements_list = [mlrun_version] + requirements_list | |
requirements_list.insert(0, mlrun_version) |
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.
funny, that's what I did at first but then I thought that the other way is more readable.
fixed
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 pr and nice test coverage overall.
But I think as we already discussed in your previous PR, there is a scenario where a user can pass with_mlrun=True
but we change it to with_mlrun=False
if we see that the image is one of ours (mlrun/mlrun or mlrun/ml-models
etc..). Now I think that we should add the mlrun requirement any way if there are other requirements as well as part of the build so that the requirements will be installed all together.
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, though the frontend-spec
is changed here, so I'm a bit concerned - why is it in the frontend-spec
to begin with? Could it be someone is using it and may break due to this change?
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.
👍 sababa
We need to install mlrun in the same command as with all other python packages so that pip will take into consideration the mlrun requirements.
Implementation: