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

omnibus-2024-08-02 #557

Merged
merged 25 commits into from
Aug 3, 2024
Merged

Conversation

maddenp-noaa
Copy link
Contributor

@maddenp-noaa maddenp-noaa commented Aug 2, 2024

Synopsis

  1. Simplify access to drivers' internal config. Previously, we had .config, .config_full, and ._driver_config properties. .config provided the block including the driver's own block and the platform block (i.e. {"driver-name": ..., "platform": ...}); .config_full provided the full input config, even content not relevant to the driver; ._driver_config provided the block under driver-name. This PR simplifies this: The private instance variable ._config_full now provides the {"driver-name": ..., "platform": ...} block, and .config_full provides a public read-only copy of that; and the private instance variable ._config now provides the block under driver-name, and .config provides a public read-only copy of that. So, .config replaces the old ._driver_config, and .config_full replaces the old .config. The driver no longer retains a copy of the complete input config, discarding the irrelevant parts of that input. Many references to these properties are updated to make use of this simpler arrangement.

  2. Update uwtools.api.driver to explicitly import the Assets and Driver class families, instead of dynamically adding these to the module. I found in starting external coupled-driver development that running pylint on that driver code flagged e.g. from uwtools.api.driver import DriverCycleBased, reporting that uwtools.api.driver has no DriverCycleBased member. From a static analysis perspective, this is true: The class is added to the module only when it is imported and its top-level code executed, so pylint cannot know this. Without this change, all external driver developers would have to suppress complaints from static analyzers like pylint.

  3. Refactor the key-path walking and error-checking code to a public function, and call it from the Assets constructor so that key-path walking there benefits from error checking and reporting.

  4. Add public .cycle and .leadtime properties on the appropriate driver classes. I see no reason to hide these, and where external drivers need to access these values, using e.g. self.cycle is IMO less surprising than self._cycle.

  5. Remove some .mkdir() calls where they are no longer necessary when they rely on downstream code that already makes necessary parent directories.

  6. I recently realized that Path() accepts multiple arguments, similar to the legacy os.path.join, so I've simplified some calls like Path(a) / b to Path(a, b). It's only one character shorter, but IMO looks simpler and more familiar.

  7. Fix some API docs.

See some inline comments, too.

Type

  • Code maintenance (refactoring, etc. without behavior change)
  • Documentation

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 2, 2024
src/uwtools/api/driver.py Show resolved Hide resolved
src/uwtools/api/driver.py Show resolved Hide resolved
src/uwtools/api/driver.py Show resolved Hide resolved
src/uwtools/config/tools.py Show resolved Hide resolved
src/uwtools/drivers/cdeps.py Show resolved Hide resolved
src/uwtools/drivers/driver.py Show resolved Hide resolved
src/uwtools/tests/drivers/test_driver.py Show resolved Hide resolved
src/uwtools/py.typed Outdated Show resolved Hide resolved
src/uwtools/tests/config/test_tools.py Show resolved Hide resolved
src/uwtools/tests/drivers/test_cdeps.py Show resolved Hide resolved
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.

LGTM. Just some minor suggestions.

src/uwtools/api/driver.py Show resolved Hide resolved
src/uwtools/drivers/cdeps.py Show resolved Hide resolved
src/uwtools/drivers/cdeps.py Show resolved Hide resolved
src/uwtools/drivers/driver.py Show resolved Hide resolved
src/uwtools/drivers/mpas.py Show resolved Hide resolved
src/uwtools/tests/drivers/test_driver.py Show resolved Hide resolved
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.

Finally got through everything, LGTM!

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.

LGTM!

src/uwtools/drivers/driver.py Show resolved Hide resolved
@maddenp-noaa maddenp-noaa merged commit 2158ca8 into ufs-community:main Aug 3, 2024
2 checks passed
@maddenp-noaa maddenp-noaa deleted the omnibus-2024-08-02 branch August 3, 2024 02:51
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.

4 participants