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

Operations queue #191

Merged
merged 12 commits into from
Jul 25, 2022
Merged

Operations queue #191

merged 12 commits into from
Jul 25, 2022

Conversation

v1kko
Copy link
Contributor

@v1kko v1kko commented Jul 21, 2022

Implementation status:

  • create
  • init
  • submit
  • plot
  • move operations queue deeper into the System (and not in the "top" level functions)
  • legacy --dry-run option removed
  • Fix the tests

@stefsmeets
Copy link
Collaborator

stefsmeets commented Jul 21, 2022

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}')

@v1kko
Copy link
Contributor Author

v1kko commented Jul 22, 2022

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

@v1kko v1kko marked this pull request as ready for review July 22, 2022 12:42
@v1kko v1kko requested a review from stefsmeets July 22, 2022 12:42
@v1kko
Copy link
Contributor Author

v1kko commented Jul 22, 2022

@stefsmeets , what do you think, is this a better way to implement dry-runs et al.?

Copy link
Collaborator

@stefsmeets stefsmeets left a 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 :-)

duqtools/operations.py Show resolved Hide resolved
duqtools/operations.py Outdated Show resolved Hide resolved
duqtools/operations.py Outdated Show resolved Hide resolved
duqtools/operations.py Outdated Show resolved Hide resolved
duqtools/operations.py Outdated Show resolved Hide resolved
duqtools/operations.py Show resolved Hide resolved
tests/dry_run.yaml Show resolved Hide resolved
@v1kko
Copy link
Contributor Author

v1kko commented Jul 22, 2022

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.

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 return values through references in the arguments, which then could be picked up by the following functions in the queue (if they have the same reference as argument). Or just using global variables of course, that would work

@v1kko
Copy link
Contributor Author

v1kko commented Jul 22, 2022

I just realized I am just reinventing dask delayed https://docs.dask.org/en/stable/delayed.html 😅

@v1kko v1kko merged commit 86dadcf into main Jul 25, 2022
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.

2 participants