-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add project support in jobs #2955
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2955 +/- ##
==========================================
+ Coverage 73.63% 73.69% +0.06%
==========================================
Files 70 70
Lines 12500 12502 +2
Branches 2303 2302 -1
==========================================
+ Hits 9204 9213 +9
+ Misses 2975 2973 -2
+ Partials 321 316 -5
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
neuro-sdk/src/neuro_sdk/_jobs.py
Outdated
) -> JobDescription: | ||
url = self._config.api_url / "jobs" | ||
if not project_name: | ||
project_name = self._config.project_name or self._config.username |
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.
I am not entirely sure about this. we created default projects for existing users just once. any new users will have to create their projects manually. thus the names of the projects may not be equal to user names (there's a chance of conflicts we discussed earlier). so self._config.username
might be confusing as it may result in permission errors. additionally, we already handle this on the backend side. wdyt?
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.
Yes, API wouldn't allow users to create jobs not in their projects or w.o. projects at all, we are safe on the backend side.
As for SDK/CLI, we could either make the project_name
parameter of Jobs.run
and Jobs.start
obligatory, OR rely on self._config.project_name
and raise an exception at _job_to_api
stating smth around "Project is not specified, make sure you have created one, or reload configuration before running the jobs". I'm more for 2nd approach, wdyt?
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.
From a user experience point of view, 2 is preferred. Most users rarely switch projects, and having to constantly retype even though it rarely changes would be a hassle.
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.
This is similar in a way to clusters - most users will never access more than one
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.
these all seem valid points. my only concern is us explicitly using self._config.username
. by the time this code executes all existing users will have their default project created and even pulled from /config
. otherwise asking to create a project seems fine.
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.
in 890658a I've added one extra propery of config
named project_name_or_raise
which raises exception
The current project is not selected. Please create one with 'neuro admin add-project', or switch to the existing one with 'neuro config switch-project'.
So this property to start a job if no project
parameter is specified.
I'm not using project_name property directly, since when we add the user to the cluster, it won't have any project yet.
wdyt, @dalazx ?
bc9b7ec
to
7207a4d
Compare
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.
👍
for more information, see https://pre-commit.ci
5859b4b
to
4d11f36
Compare
for more information, see https://pre-commit.ci
closes #2948
Some assumptions made: