Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ORCA-163] Implement launch_workflow() and LaunchInfo dataclass #15

Merged
merged 13 commits into from
May 1, 2023

Conversation

BrunoGrandePhD
Copy link

@BrunoGrandePhD BrunoGrandePhD commented Apr 27, 2023

I was initially going to port the launch_workflow() implementation from sagetasks (source), but I decided to refactor it. Its reliance on the init_launch_workflow_data() method is clunky. I also didn't like the large number of function parameters.

To address this, I implemented the LaunchInfo dataclass. Its goal is to encapsulate the information related to launching a workflow. It essentially operates like an OpenAPI model. This results in much cleaner client code. This class can eventually be reused for get_workflow().

I'm marking this PR as a draft because I would like to tackle the following subtasks:

  • Implement get_compute_env() in the client class
  • Implement get_latest_compute_env() in the ops class
  • Implement higher-level launch_workflow() in the ops class
    • This version would use the default values from the compute environment (e.g. work directory) when launching a workflow (example)
    • This version would ideally be idempotent, if possible

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #15 (1069c75) into main (db0785f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main       #15    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           21        22     +1     
  Lines          434       626   +192     
  Branches        62        97    +35     
==========================================
+ Hits           434       626   +192     
Impacted Files Coverage Δ
src/orca/services/nextflowtower/client.py 100.00% <100.00%> (ø)
src/orca/services/nextflowtower/models.py 100.00% <100.00%> (ø)
src/orca/services/nextflowtower/ops.py 100.00% <100.00%> (ø)
src/orca/services/nextflowtower/utils.py 100.00% <100.00%> (ø)

@BrunoGrandePhD BrunoGrandePhD marked this pull request as ready for review April 28, 2023 23:06
@BrunoGrandePhD BrunoGrandePhD requested review from BWMac, a team and thomasyu888 April 28, 2023 23:06
Copy link
Collaborator

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for putting this together.

@BrunoGrandePhD BrunoGrandePhD requested a review from thomasyu888 May 1, 2023 19:46
@BrunoGrandePhD
Copy link
Author

Thank goodness for the test suite! I was able to resolve multiple merge conflicts with confidence.

Copy link
Collaborator

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛸 Excellent! Just a minor comment about the json variable, but going to pre-approve.

@BrunoGrandePhD BrunoGrandePhD merged commit 74f9c5c into main May 1, 2023
@BrunoGrandePhD BrunoGrandePhD deleted the bgrande/ORCA-163/tower-launch-workflow branch May 1, 2023 21:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants