Skip to content

Conversation

@phellipecouto
Copy link
Collaborator

@phellipecouto phellipecouto commented Aug 26, 2025

Enabled optional_config_files provision through config.yaml. Prior to this change, ROMS varinfo file was being considered a mandatory model config file and mistakenly hard coded into config_files list, leading to model initialisation failure when file name differed from varinfo_seacofs.yaml.

@phellipecouto phellipecouto marked this pull request as draft August 26, 2025 04:31
@coveralls
Copy link

coveralls commented Aug 26, 2025

Coverage Status

coverage: 59.162% (+0.08%) from 59.084%
when pulling 270ea77 on ACCESS-NRI:feature/roms-driver
into e6cef2d on payu-org:master.

@phellipecouto phellipecouto changed the title fix: properly handle provision of optional configuraiton files fix: properly handle provision of optional configuration files Aug 26, 2025
@phellipecouto phellipecouto self-assigned this Aug 26, 2025
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Given this is a fairly significant departure from how optional config files are handled it would be good to have this documented.

We already have an open issue for the lack of documentation of drivers

#470

How is your RST @phellipecouto? Would you be willing to be the first to add driver docs?

aidanheerdegen
aidanheerdegen previously approved these changes Aug 26, 2025
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

LGTM. We can deal with docs later ...

@phellipecouto
Copy link
Collaborator Author

Given this is a fairly significant departure from how optional config files are handled it would be good to have this documented.

We already have an open issue for the lack of documentation of drivers

#470

How is your RST @phellipecouto? Would you be willing to be the first to add driver docs?

OK good to hear this! I had a slight impression I could be going on a different direction as I could not find a working example of this on other drivers. Can you point me to the right direction and I will assess if this would still work for the ROMS case?

@aidanheerdegen
Copy link
Collaborator

Can you point me to the right direction and I will assess if this would still work for the ROMS case?

I think what you've done is fine. Off the top of my head I can't think of any other drivers that have dynamic model config files, so I think it's fine.

I'll start a separate PR to add driver docs for more of the models, and get your input for ROMS then.

@aidanheerdegen aidanheerdegen marked this pull request as ready for review August 27, 2025 01:24
@aidanheerdegen aidanheerdegen merged commit 0fd643d into payu-org:master Aug 27, 2025
8 checks passed
@aidanheerdegen aidanheerdegen deleted the feature/roms-driver branch August 28, 2025 01: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.

3 participants