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

Load neps arguments from a .yaml file using a new argument run_args #61

Merged
merged 29 commits into from
Apr 30, 2024

Conversation

danrgll
Copy link
Contributor

@danrgll danrgll commented Mar 11, 2024

  • Integrating run_args, loading and checking settings from yaml file provided via run_args
  • Tests for functionality
  • Example usage in neps examples
  • Documentation

@danrgll danrgll added the enhancement New feature or request label Mar 11, 2024
@danrgll danrgll changed the title Load neps arguments from a .yaml file using a new argument run_args [WIP]Load neps arguments from a .yaml file using a new argument run_args Mar 11, 2024
@danrgll danrgll changed the title [WIP]Load neps arguments from a .yaml file using a new argument run_args Load neps arguments from a .yaml file using a new argument run_args Mar 21, 2024
@danrgll danrgll requested a review from Neeratyoy March 21, 2024 19:10
@danrgll danrgll mentioned this pull request Apr 25, 2024
12 tasks
Copy link
Contributor

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

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

Overall, some minor comments and I did not review the documentation.

I think you put a bit too much effort in documenting everything in the tests which is nice but save your effort for other parts!

The main things I identified to move towards merging this would be first to just explicitly not allow merging yaml parameters and function parameters. Yes it might be nice but not worth blocking on. We can revisit if necessary. Please raise an issue referencing this PR. You can use #<number> to refer to a specific issue or PR.

# text...

Please see PR #61

@eddiebergman
Copy link
Contributor

Heyo, please let me know when you'd be ready for another review and we can get to work mergining this and cleaning anything up in a follow up PR!

@danrgll
Copy link
Contributor Author

danrgll commented Apr 29, 2024

I implemented the strict check, from my side we can merge this now. Then I will create a new branch on top of this for refactoring.

@eddiebergman eddiebergman merged commit 656f80c into master Apr 30, 2024
22 checks passed
@eddiebergman eddiebergman deleted the run_pipeline_yaml branch April 30, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants