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

Fix agent update version #132

Merged
merged 3 commits into from Jan 8, 2023
Merged

Fix agent update version #132

merged 3 commits into from Jan 8, 2023

Conversation

achaiah
Copy link
Contributor

@achaiah achaiah commented Dec 21, 2022

Pip install command is missing the '==' to execute successfully

Pip install command is missing the '=='  to execute successfully
@jkhenning
Copy link
Member

Hi @achaiah,

Thanks for your contribution!
Actually, the current implementation has its rational - it allows you to specify both exact versions (by setting CLEARML_AGENT_UPDATE_VERSION="==1.2.3" as well as other constraints (e.g. CLEARML_AGENT_UPDATE_VERSION="<=1.2.3")

@achaiah
Copy link
Contributor Author

achaiah commented Dec 30, 2022

Hmm... this is an "interesting" solution as I wouldn't have expected this format when providing an ENV variable. What's worse, it doesn't really produce any errors when you simply provide the version without the "==" which is what threw me for a loop in the first place.

I expected to be able to provide just the version number. Is this behavior documented anywhere? I had to dig through the source code to figure it out but I'm new to ClearML.

@jkhenning
Copy link
Member

I'm not sure it's documented, but maybe the best thing to do is to simply add the option to provide a simple version as well (for example testing if the string is in "d.d" or "d.d.d" format and if so, append "=="?

adding logic to allow for flexible version specification
@achaiah
Copy link
Contributor Author

achaiah commented Jan 4, 2023

Ok I think your suggestion would work. Please take a look at the regex that I added.

@jkhenning
Copy link
Member

Looks good. Maybe you'd like to add support there for RC versions (in the format 1.2.3rc7)?

@achaiah
Copy link
Contributor Author

achaiah commented Jan 8, 2023

Ugh..lol yeah, the regex gets more complicated but I think I got it

@jkhenning jkhenning merged commit ebb9551 into allegroai:master Jan 8, 2023
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

2 participants