-
Notifications
You must be signed in to change notification settings - Fork 3
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
Operations queue #191
Operations queue #191
Conversation
Hi @v1kko this looks really nice! I was just wondering if it would be clearer to pass the function to call along with any arguments / keyword arguments instead of lambdas? op_queue.add(
action=system.copy_from_template,
args=(template_drc, run_drc),
kwargs={'kwarg1': 123},
description=f'Copying template to {run_drc}'
) Instead of: op_queue.add(action=lambda run_drc=run_drc: system.copy_from_template(
template_drc, run_drc),
description=f'Copying template to {run_drc}') |
Definitely! Also gets rid of the lambda resolving issue |
@stefsmeets , what do you think, is this a better way to implement dry-runs et al.? |
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.
Cool! Seems to work well, but it's not entirely clear to me how this works.
From my understanding, this only works for functions that do not return anything right? So if they make changes to the file system or submit jobs that is fine. But if a function returns something that is needed by the next function this will no longer work.
I do think it makes the 'dry-run' clearer. Actually, I'm quite impressed how you got this to work using only decorators.
I also think we should work on cleaning up the output to the terminal (not in this PR), because it is difficult to see what you are actually confirming.
I left some nitpicks in the comments :-)
Yes that is one downside of this system, because functions are delayed and put into a queue until the end of the program, there is nothing to return to. It would still be possible (but ill-advised) to |
I just realized I am just reinventing dask delayed https://docs.dask.org/en/stable/delayed.html 😅 |
Implementation status: