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

Add ability to specify duplicate command names in Pipeline #174

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

ppinchuk
Copy link
Collaborator

@ppinchuk ppinchuk commented Nov 4, 2023

Short example on how to specify duplicate pipeline steps is here.

@ppinchuk ppinchuk marked this pull request as ready for review November 4, 2023 22:16
@ppinchuk ppinchuk linked an issue Nov 4, 2023 that may be closed by this pull request
@ppinchuk ppinchuk requested a review from bnb32 November 4, 2023 22:17
Copy link
Collaborator

@bnb32 bnb32 left a comment

Choose a reason for hiding this comment

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

Looks great, man! Seems like this was just a bit tedious but easier than expected? Based on the test it looks like pipeline_step doesn't need to be specified explicitly, but I'm not seeing where in the code the implicit specification is handled?

@ppinchuk
Copy link
Collaborator Author

ppinchuk commented Nov 5, 2023

Yea, you're 100% right about it being easier than expected. When I was ding the GAPs refactor, I realized it would be a non-negligible amount of changes to integrate this feature, which I had no idea if you guys needed or even wanted. I think at that time I decided to take the conservative approach and told myself "sup3r will just behave as if its still running on the reV pipeline". I made a mental note of that and moved on, which is probably why I was remembering that it would be "difficult". Now that there's appetite for this, I think the 100 extra lines are totally worth it.

The pipeline_step parsing is handled by GAPs, which then passes it as an argument to the from_config functions in sup3r. The parsing step is super basic, so let me know if you have any better ideas for how to specify these in the config and read them into the code.

@bnb32
Copy link
Collaborator

bnb32 commented Nov 6, 2023

@ppinchuk Awesome, thanks for knocking this out so quickly. The parsing looks good to me!

@ppinchuk ppinchuk merged commit 943b6b9 into main Nov 6, 2023
8 checks passed
@ppinchuk ppinchuk deleted the pp/duplicate_commands branch November 6, 2023 17:43
github-actions bot pushed a commit that referenced this pull request Nov 6, 2023
Add ability to specify duplicate command names in Pipeline
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.

sup3r status file updates use command name not unique key name
2 participants