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

[DRAFT] Config Parser #282

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[DRAFT] Config Parser #282

wants to merge 7 commits into from

Conversation

annaelisalappe
Copy link
Collaborator

@annaelisalappe annaelisalappe commented Jan 8, 2025

Summary

The main contribution of this PR is the change from the old ConfigParser (which was based on jsonargparse) to a new one, based on hydra. The advantage of this change is that error messages that arise during execution of the pipeline were often not visible in the error message given out by the jsonargparse parser, whereas in hydra they are passed on. This makes debugging much easier. An example of the difference:

If I set the seed in the config file to "small", I get the following outputs from the old parser and the new one, respectively:

Screenshot 2025-01-08 at 16 50 20

Screenshot 2025-01-08 at 16 51 42

Some other changes that were made in the course of this addition:

  • I merged the function parse_pipeline and parse_step into one function called build_from_config, as they were almost identical after the change
  • I made it so that the config can only be passed as a path to a file, because it seemed that there is no use case where passing it as a dictionary was actually used. If it should still be possible to pass a dictionary, I would pass it as a second attribute to keep the code cleaner

annaelisalappe and others added 4 commits January 7, 2025 18:02
…rged methods parse_pipeline and parse_step into one function build_from_config
* add slurm template script

* add eurac slurm script for testing

* fix some linting errors

* add contributors

* make script class based

* add code for running script

* fix more linting errors

* move slurm scripts into src and add use-case specific script

* rename slurmscript class

* add argparser

* create runall methods

* fix import order

* add builder for tutorial

* make parser more general

* add logic for disabling running script

* add function for generating identifier

* fix linting errors

* make runall work for eurac

* fix merge conflicts

* some code clean up

* small adjustments to make it work for the EURAC case

* add implementation for virgo use case

* add slurm_scripts/ to gitignore

* add ability to override training command in config file

* update std_out and err_out automatically

* add auto std_out for real this time

* make slurm script for scaling test

* fix linting errors

* add separate file for generic slurm script maker and small changes

* fix linting errors
@annaelisalappe annaelisalappe self-assigned this Jan 8, 2025
@annaelisalappe annaelisalappe added the enhancement New feature or request label Jan 8, 2025
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.

2 participants