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

Fix issues with shared config files #157

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

xylar
Copy link
Collaborator

@xylar xylar commented Dec 1, 2023

This merge fixes two main issues with shared config files:

  1. The main one is adding an explicit attribute of steps to indicate whether they have a shared config (the alternative being that they use the config for the task they belong to). Previously, a step was assumed to have a shared config if it belonged to multiple tasks but that causes trouble when a conceptually shared step happens to belong to only one task (e.g. because other tasks may use it in the future).
  2. The minor fix is that only the tasks and steps requested during setup have their config files linked. Previously, symlinks to the shared config were being created for all tasks and steps that used the config, even if they were not requested at setup. This created confusing directories for unrequested tasks that just had a symlink to the config file.

Checklist

  • Testing comment in the PR documents testing used to verify the changes

This gets set to `True` when `set_shared_config()` gets called.
Remove unused list of shared steps.
We only want to symlink config file for requested tasks
We get the symlinks from the requested task and steps instead
so we don't create symlinks to configs for tasks and steps that
weren't requested.
@xylar xylar added bug Something isn't working framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis labels Dec 1, 2023
@xylar xylar self-assigned this Dec 1, 2023
@xylar
Copy link
Collaborator Author

xylar commented Dec 1, 2023

Testing

I ran all ocean test cases except the 1 and 4 km baroclinic channel RPE tests (which are too expensive) on Chrysalis with Intel and OpenMPI. All tests were successful except manufactured solution, which failed the usual verification test.

I also tested #141 after rebasing onto this branch and it fixed the issues with the inception, wetting and drying tests that had previously been seen, see #141 (comment)

@xylar xylar requested a review from cbegeman December 1, 2023 17:39
@xylar
Copy link
Collaborator Author

xylar commented Dec 1, 2023

@cbegeman, can you just give this a quick review, probably just by inspection since I think my testing is sufficient?

Copy link
Collaborator

@cbegeman cbegeman left a comment

Choose a reason for hiding this comment

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

@xylar Great, thanks! I had noticed the second issue so it's nice that you included it here. Approving by visual inspection.

@xylar
Copy link
Collaborator Author

xylar commented Dec 1, 2023

Great, thanks for the rapid response, @cbegeman!

@xylar xylar merged commit 58e6c4d into E3SM-Project:main Dec 1, 2023
4 checks passed
@xylar xylar deleted the fix-shared-configs branch December 1, 2023 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to ocean tests or analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants