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

Remove assumption that $modeloutdir == $outdir / "out" #3439

Open
infotroph opened this issue Feb 13, 2025 · 2 comments
Open

Remove assumption that $modeloutdir == $outdir / "out" #3439

infotroph opened this issue Feb 13, 2025 · 2 comments

Comments

@infotroph
Copy link
Member

Me in code comments of ccmmf/workflows#1 :

<!-- TODO at least one function hardcodes an assumption that model output
   lives in `file.path(settings$outdir, "out", runid)`. We should fix this,
   but meanwhile name your settings$host$outdir by adding
   "/out" to the value in settings$outdir. -->

Resulting discussion:

dlebauer on Jan 8

Here are all of the occurrences in R/*R files (excluding inst, workflows, Rscripts, tests). https://github.com/search?q=repo%3APecanProject%2Fpecan+%22settings%24outdir%2C+%5C%22out%5C%22%22+path%3AR%2F*.R&type=code

It looks like this assumption is built into / enforced in check.settings. If the config doesn't include a modeloutdir it will be created.

I think the fix would be to replace file.path(settings$outdir, "out"), with settings$modeloutdir in these two files.

file.path(settings$outdir, "out", run.id),

path = file.path(settings$outdir, "out"),

If that's correct, I can create an issue. How important is this fix for CCMMF?

@infotroph last month

Not important to fix for CCMMF.

replacing file.path(settings$outdir, "out"), with settings$modeloutdir seems reasonable but should include making sure the differences are documented in the XML chapter -- I'm still fuzzy on what should use outdir vs modeloutdir vs host$outdir.

@mdietze last month

Agree that file.path(settings$outdir, "out") should only be hard coded in the settings check as the default for modeloutdir if a specific value is not provided.

In terms of use of these file path variables, outdir is the main workflow folder, modeloutdir is where workflow-specific model output is stored and defaults to file.path(settings$outdir, "out"), rundir is analogous for inputs/configs and defaults to file.path(settings$outdir, "run"), and the analogous paths in the host section is where these paths would be located on a remote host.

@infotroph infotroph changed the title Remove assumption that Remove assumption that $modeloutdir == $outdir / "out" Feb 13, 2025
@Sweetdevil144
Copy link
Contributor

Sweetdevil144 commented Feb 13, 2025

Wouldn't settings inherit the outdir value from the yml xml (edit) file? Am I on the right path here?

If I'm correct and that's how settings work then we can simply assign it via the parsed yml xml (edit) tree as we do with other structures like in all of the above recommended lines. Also, would it work if we replace the above lines with a specific path like :

read.output( ... , settings$modeloutdir)
# After we inherit settings from check.all.settings

@infotroph
Copy link
Member Author

Wouldn't settings inherit the outdir value from the yml file?

Yes, that's the intended behavior (well, XML not YML), and the bug is that these are places we're hardcoding a value that ignores any user-provided modeloutdir. Very simple fix if we can be sure modeloutdir is already set at those moments and can be used unconditioanlly, pretty simple if we also need to handle providing a default when modeloutdir is unset (in which case outdir/"out" seems like a fine default to use).

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

No branches or pull requests

2 participants