-
Notifications
You must be signed in to change notification settings - Fork 7
add job submition dependency option #313
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
base: main
Are you sure you want to change the base?
add job submition dependency option #313
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.
I feel like the _build_dependency_string method could be way lighter, e.g.:
@staticmethod
def _build_dependency_string(
depend_on: Job | list[Job] | str | list[str],
dependency_type: str = "afterok",
) -> str:
depend_on = depend_on if isinstance(depend_on, list) else [depend_on]
if job := next(
(j for j in depend_on if isinstance(j, Job) and not j.job_id),
None,
):
msg = f"Job '{job.name}' has not been submitted yet."
raise ValueError(msg)
return f"{dependency_type}:" + ":".join(
dependency.job_id if type(dependency) is Job else dependency
for dependency in depend_on
)or, with my comment below about the status check rather than job_id:
@staticmethod
def _build_dependency_string(
depend_on: Job | list[Job] | str | list[str],
dependency_type: str = "afterok",
) -> str:
depend_on = depend_on if isinstance(depend_on, list) else [depend_on]
if job := next(
(j for j in depend_on if isinstance(j, Job) and j.status.value < JobStatus.QUEUED.value),
None,
):
msg = f"Job '{job.name}' has not been submitted yet."
raise ValueError(msg)
return f"{dependency_type}:" + ":".join(
dependency.job_id if type(dependency) is Job else dependency
for dependency in depend_on
)And, if the check for f{dependency_type}: is really needed (see my comment on the corresponding test below), you can still filter it with something like:
@staticmethod
def _build_dependency_string(
depend_on: Job | list[Job] | str | list[str],
dependency_type: str = "afterok",
) -> str:
depend_on = depend_on if isinstance(depend_on, list) else [depend_on]
depend_on = [
dependency.lstrip(f"{dependency_type}:")
if isinstance(dependency, str)
else dependency
for dependency in depend_on
]
if job := next(
(
j
for j in depend_on
if isinstance(j, Job) and j.status.value < JobStatus.QUEUED.value
),
None,
):
msg = f"Job '{job.name}' has not been submitted yet."
raise ValueError(msg)
return f"{dependency_type}:" + ":".join(
dependency.job_id if type(dependency) is Job else dependency
for dependency in depend_on
)but it already starts to be messy again and I think I'd use a proper helper method to set the depend_on list up 🥸🥸.
Also, all your tests could higlhy benefit from parametrizing, you have lots of repeated code chunks!
|
I tried to make better tests, let me know your thoughts @Gautzilla |


I added the possiblity to run a job instance after another job is finished
This can be used either using a string corresponding to the first job id or another Job instance
Note:
Static method
_build_dependency_stringfromJobclass implement by default "afterok" dependency but other dependecies have already been implemented altough they are not used yet