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

[Projects] Fix second create_remote not overriding current remote #5245

Merged
merged 7 commits into from Mar 18, 2024

Conversation

yaelgen
Copy link
Collaborator

@yaelgen yaelgen commented Mar 5, 2024

resolves: ML-5079

@yaelgen yaelgen marked this pull request as ready for review March 5, 2024 19:02
Copy link
Member

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

Looking good!

I think there is no real reason to have both create_remote and set_remote, as they provide the same functionality but with an extra feature in set.
We can maybe deprecate create_remote (add deprecation warning and remove in 2 versions), and encourage users to use only set_remote.

tests/projects/test_project.py Outdated Show resolved Hide resolved
@TomerShor TomerShor changed the title [Projects] Fixed second create_remote not overriding current remote [Projects] Fix second create_remote not overriding current remote Mar 7, 2024
Copy link
Member

@quaark quaark left a comment

Choose a reason for hiding this comment

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

Spoke with @yaelgen offline, I feel a better solution instead of changing create to set (with override defaulting to False), we should extend the API to include create (never overrides), set (override defaults to True and can be set to False) and remove

mlrun/projects/project.py Outdated Show resolved Hide resolved
mlrun/projects/project.py Outdated Show resolved Hide resolved
mlrun/projects/project.py Show resolved Hide resolved
mlrun/projects/project.py Outdated Show resolved Hide resolved
Copy link
Member

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@quaark quaark left a comment

Choose a reason for hiding this comment

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

Great stuff!

@liranbg liranbg merged commit 7218958 into mlrun:development Mar 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants