Updated the yaml output to reference the toml file in the output#4264
Updated the yaml output to reference the toml file in the output#4264AmanKashyap0807 wants to merge 1 commit intoCliMA:mainfrom
Conversation
There was a problem hiding this comment.
The following contributor(s) must sign the CLA before this PR can be merged:
Please visit https://ecodesign.clima.caltech.edu/cla/ to review and sign the CLA.
How to sign: Authenticate with GitHub then click the "I agree" button.
Once completed, re-run the checks on this PR.
There was a problem hiding this comment.
Thank you again for working on this, @AmanKashyap0807!
I left a comment with minor improvements for you to consider.
Could you also write a unit test that verifies this behavior? It may be easiest to make a new file, as I don't see any existing tests call get_simulation.
You'll also need to sign the contributor license agreement, if you haven't done so already.
@nefrathenrici This PR makes me realize that because this functionality is contained within get_simulation, it is not called when we create the simulation through a runscript (ref #4236); as is the case for YAML.write_file. I wonder if it would be cleaner if the functionality is put into a separate function that both get_simulation and AtmosSimulation calls. What do you think?
src/solver/type_getters.jl
Outdated
| @@ -995,7 +995,11 @@ function get_simulation(config::AtmosConfig) | |||
| joinpath(output_dir, "$(job_id)_parameters.toml"), | |||
There was a problem hiding this comment.
It looks like you could make a variable name like:
output_toml_file = joinpath(output_dir, "$(job_id)_parameters.toml")and reference that both in the call to CP.log_parameter_information, and the code you wrote below (but with abspath(output_toml_file). That way, it's slightly clearer that we're referring to the same file.
There was a problem hiding this comment.
Thanks for the suggestion, That makes sense
I’ll update the code to introduce output_toml_file and reuse it in both places.
|
I have added a unit test that run minimal simulation using Please let me know if you’d like any changes to the test or implementation. I’m also happy to help with other issues. |
haakon-e
left a comment
There was a problem hiding this comment.
Thanks for the updates! A few comments about tests, then this should be good to go.
|
Hi @haakon-e, The first commit simplifies the test_output_yaml_path test and adds it to |
|
@AmanKashyap0807 Thanks! Can you rebase to latest main and squash your changes into one commit? |
…ded test_output_yaml_path for it and added it in CI
directory to prevent restart failures when original files are moved
Purpose
This PR addresses issue #4040. Currently, the output YAML file generated after a simulation references the paths of the original input TOML files. If those original files are moved or deleted, restarting the simulation from the output YAML fails.
This change ensures that the output YAML points to the absolute path of the parameters TOML file that is automatically saved in the output directory ( <job_id>_parameters.toml ). This makes the output folder self-contained and allows for robust restarts even if the original input files are modified or removed.
To-do
get_simulationin type_getters.jl to update the toml path in output configContent
The fix is implemented in
type_getters.jlwithing the get_simulation function.Prior to writing the output YAML file, I created a copy of config.parsed_args to avoid modifying the runtime configuration object (which seemed safer than in-place mutation). In this copy, I updated the toml key to be a list containing the absolute path to the local
*_parameters.tomlfile.As suggested by @haakon-e and @nefrathenrici , I used the full absolute path (abspath(...)) for the new TOML entry. This avoids potential ambiguity with relative paths depending on where the Julia process is launched.
Verification Step:
I verified this manually using a small "box" simulation configuration:
Let me know if any adjustment are needed, or if you want any test script.