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

Refactor prep config #336

Merged
merged 34 commits into from
Jun 12, 2024
Merged

Refactor prep config #336

merged 34 commits into from
Jun 12, 2024

Conversation

Dooruk
Copy link
Collaborator

@Dooruk Dooruk commented May 7, 2024

Same thing as #324 that @danholdaway introduced with additional fixes.

This shouldn't change (and didn't change as far as I could test) any of the configuration YAML outputs. I tested creating experiments with the defaults/overrides and Callum and I tested the CLI interface.

A few highlights:

  • It replicates the task_questions logic with the suite_questions so we don't have to have suite_questions in each suite folder.
  • It changes Jinja2 utility slightly to add a custom "undefined" class, which essentially gives Jinja2 an option to skip certain template variables without breaking.
  • It moves R2D2 config creation to utilities but again, doesn't change the configuration.
  • This should help with the model dependent tasks (e.g., a variable change task only required for CICE)

I made a few minor fixes and merged with SLURM changes by @ashiklom (I had to add dashes to avoid whitespace between SLURM directives and added items to jinja2 undefined class).

So please take a look and test if you can and see if anything breaks. I'm planning on merging this next week.

@Dooruk Dooruk changed the title Feature/refactor prep config Refactor prep config (Doruk's version) May 8, 2024
@Dooruk Dooruk requested a review from rtodling May 8, 2024 17:21
@Dooruk Dooruk marked this pull request as ready for review May 8, 2024 17:40
@Dooruk Dooruk requested a review from CRWayman May 9, 2024 15:35
@ashiklom
Copy link
Collaborator

ashiklom commented Jun 7, 2024

@Dooruk I gave this a pretty thorough review. Functionally, I think it looks good and I didn't find any bugs. My suggestions above are primarily related to style and, in a few cases, adding additional robustness or clarity.

That said, don't let my suggestions hold up this PR. Take a quick look through them and feel free to either directly commit them (I tried to make it a one-click thing using "Suggestions") or just "resolve" the conversation without implementing the changes. Especially for the type hinting stuff (#349), I can always just open a separate PR on that later.

@Dooruk
Copy link
Collaborator Author

Dooruk commented Jun 12, 2024

Thanks for your thorough review @ashiklom, I made most of the modifications you suggested and deferred some to a future PR. This is almost set to merge, except while I started fixing some issues with the CLI I encountered an error with the iso-duration type:

Screenshot 2024-06-12 at 9 54 11 AM

So make_duration in question_and_answer_cli is explicitly set to handle PT12H type entries but not D as I quickly looked at it, which should be an easy fix. I have a few meetings back-to-back now but if anyone has time to fix that that would be great.

@Dooruk Dooruk requested a review from ashiklom June 12, 2024 14:15
Copy link
Collaborator

@ashiklom ashiklom left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Dooruk Dooruk merged commit 706a10b into develop Jun 12, 2024
11 checks passed
@Dooruk Dooruk deleted the feature/refactor_prep_config branch June 12, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants