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 --schema-file option to internal drivers #592

Merged
merged 34 commits into from
Aug 27, 2024

Conversation

maddenp-noaa
Copy link
Contributor

Synopsis

  • Add --schema-file option to all built-in drivers.
  • Update and DRY out driver docs.

Type

  • Documentation
  • Enhancement (adds new functionality)

Impact

  • This is a non-breaking change (existing functionality continues to work as expected)

Checklist

  • I have added myself and any co-authors to the PR's Assignees list.
  • I have reviewed the documentation and have made any updates necessitated by this change.

@maddenp-noaa maddenp-noaa self-assigned this Aug 25, 2024
Copy link
Contributor

@NaureenBharwaniNOAA NaureenBharwaniNOAA left a comment

Choose a reason for hiding this comment

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

This looks great! I appreciate the refactoring to the driver docs. it really helps make it consistent and easier to update when/if needed.

Copy link
Contributor

@WeirAE WeirAE left a comment

Choose a reason for hiding this comment

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

Whew, a marathon of small but fantastic updates, LGTM

Copy link
Contributor

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

I think what is here looks great. I feel like there may still be some work to be done in the driver code.

It looks like get_schema_file is still called there for the class method schema and in the function _namelist_schema. To my knowledge, that function only looks for the uwtools-provided schema file where we might want to instead use the user-provided one, if provided.

@maddenp-noaa
Copy link
Contributor Author

@christinaholtNOAA I think the current get_schema_file() behavior is correct: It always returns the built-in schema file. I suppose it could behave either way, but I don't see any sense in giving back to the user the schema they just gave us. They have it. Maybe we can be clearer in docs/docstrings, though. I'll check.

But _namelist_schema definitely needs to be updated. We discussed it here in the context of external drivers, but there's an even greater need to update it in the context of internal drivers using external schemas. I'll open a Jira ticket for it.

@maddenp-noaa
Copy link
Contributor Author

It just occurred to me: --show-schema and --schema-file are in different parts of the CLI: The former is mode-level (uw <driver> --show-schema), while the latter is action (task) level (e.g. <driver> <task> --schema-file). So --schema-file will always show the internal schema. But we should probably also have some log.debug() statements around the schema-selection logic so that --verbose would show what's going on. I'll see about adding those when I fix the _namelist_schema() logic.

@maddenp-noaa
Copy link
Contributor Author

I updated this PR to make _namelist_schema() on class Assets public, and updated it to use an external schema if one was provided. I also added some additional debug log messages related to schema selection and use, and added some detail to existing ones to try to make them even more informative. Some docs were also updated as a result.

Copy link
Contributor

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

Thanks! Those changes LGTM. :)

@maddenp-noaa maddenp-noaa merged commit ab45222 into ufs-community:main Aug 27, 2024
2 checks passed
@maddenp-noaa maddenp-noaa deleted the schema-file-all-drivers branch August 27, 2024 13:33
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.

5 participants