Copy input files to output folder

[mjademitchell]

Description

I created a copy_input_files function that allow when simulation has ran it copies input files into an input/ subdirectory of the output folder.

I also included the copied files to set to read-only

Fixes #1109

Type of change

• Bug fix (non-breaking change to fix an issue)
• New feature (non-breaking change to add functionality)
• Refactoring (non-breaking, non-functional change to improve maintainability)
• Optimization (non-breaking change to speed up the code)
• Breaking change (whatever its nature)
• Documentation (improve or add documentation)

Key checklist

• All tests pass: $ cargo test
• The documentation builds and looks OK: $ cargo doc
• Update release notes for the latest release if this PR adds a new feature or fixes a bug
  present in the previous release

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

❌ Patch coverage is 58.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.65%. Comparing base (e8aa350) to head (ac1b168).
⚠️ Report is 46 commits behind head on main.

Files with missing lines	Patch %	Lines
src/cli.rs	54.54%	0 Missing and 5 partials ⚠️
src/output.rs	61.53%	1 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1282      +/-   ##
==========================================
- Coverage   89.79%   89.65%   -0.14%     
==========================================
  Files          57       57              
  Lines        8211     8441     +230     
  Branches     8211     8441     +230     
==========================================
+ Hits         7373     7568     +195     
- Misses        542      565      +23     
- Partials      296      308      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tsmbland]

This is a good start! We'll want an ability to turn this off, i.e. a copy_input_files option that can be true/false (true by default). I guess this should live in the program settings.toml file rather than the model.toml - @alexdewar would you agree?

Once we've got this option, we should turn it off when generating the test data with just regenerate_test_data as we don't want to clog up the repo with duplicates of all the example model input files.

[tsmbland]

@mjademitchell Side point, it would be worth running pre-commit install on your machine so it starts running the pre-commit hooks locally. Just saves the bot from having to do it on github

[alexdewar]

This looks great. V useful! I think it needs a couple of small tweaks, but otherwise all good.

When you run just regenerate_test_data now, you get all the input folders along with the test data. I think this is fine, but we don't want to commit these files, so we should add **/input/* to the tests/data/.gitignore file.

[alexdewar]

Could you also include the model name in the path, i.e. have the folder be {output_folder}/input/simple for the simple example? We get the name in this way in get_output_dir().

It would be nice to be able to run the model from the output folder, but if the folder is called input, MUSE2 will assume it's a model called input and write the output to muse2_results/input.

[tsmbland]

It would be nice to be able to run the model from the output folder

Perhaps, but to be honest I can't see a good reason to ever want to do this! We already have the results right there so no need to run again, nor should people ever be directly changing these files to run the model in a slightly different way - if you wanted to do that then better to copy the files to a different path and run them from there...

[tsmbland]

In fact, I would go one step further and ban running models in the muse2_results folder. I really don't think this is something we should be allowing/encouraging. For another PR though!

(Not opposed to the original suggestion of changing the path, I think that's fine but not necessarily essential)

[alexdewar]

It would be nice to be able to run the model from the output folder

Perhaps, but to be honest I can't see a good reason to ever want to do this! We already have the results right there so no need to run again, nor should people ever be directly changing these files to run the model in a slightly different way - if you wanted to do that then better to copy the files to a different path and run them from there...

Agree that changing the model then running it would be bad! But it's legitimate to tell MUSE2 to run the model from that folder though, I think. You might want to do this if you're trying to reproduce the outputs of a model that someone has sent you, for example. I haven't opened an issue for it, but I've been thinking it might be neat to have a --reproduce option for the run command that tries to reproduce someone's results from an output folder (reusing some of the machinery we use for regression tests).

We could make the files readonly to stop users mucking about with them as discussed: #1283

[tsmbland]

I haven't opened an issue for it, but I've been thinking it might be neat to have a --reproduce option for the run command that tries to reproduce someone's results from an output folder (reusing some of the machinery we use for regression tests).

I think that's a nice idea.

The other concern about running the input files from the results folder is that, since the name of the model will match the name of the results folder you're running it from, then if --overwrite is set to true, running this model will delete the original output files, which is probably not what you want! Further, it would delete the input files themselves so I think you wouldn't actually be able to do it unless we reorder the events in handle_run_command. You could get around this by setting --output-dir, but this is still quite a dangerous trap.

At least with the --reproduce option we could ensure that this doesn't happen

[tsmbland]

When you run just regenerate_test_data now, you get all the input folders along with the test data. I think this is fine, but we don't want to commit these files, so we should add **/input/* to the tests/data/.gitignore file.

@alexdewar I had the same comment but a different suggestion! I think this is a good idea as well/instead. I still think it might be worth making it configurable though - what do you think?

[mjademitchell]

Just reading through the comments @tsmbland @alexdewar

this is my todo;

• Ill add a true and false option for the copying of the input files
• add */input/ to the gitignore
• and change the input/ to input/{model_name}/ (right now its \MUSE2\muse2_results\simple\input)
• remove the readonly implementation

was there anything else

[alexdewar]

When you run just regenerate_test_data now, you get all the input folders along with the test data. I think this is fine, but we don't want to commit these files, so we should add **/input/* to the tests/data/.gitignore file.

@alexdewar I had the same comment but a different suggestion! I think this is a good idea as well/instead. I still think it might be worth making it configurable though - what do you think?

I'm not averse to making it configurable. I guess for consistency with the other options though, we should make it configurable in settings.toml as well as via a command-line flag. Do you agree @tsmbland?

That's a little faffier to implement (sorry, @mjademitchell!) -- but if you want an example, you can look at the debug_model option in struct Settings and struct RunOpts.

...and on that note, I'm wondering if it might be nicer if we could come up with a way that users can override arbitrary options for settings.toml with command-line flags without having all this duplicated code. Maybe Copilot has some ideas...

Just reading through the comments @tsmbland @alexdewar

this is my todo;

• Ill add a true and false option for the copying of the input files
• add */input/ to the gitignore
• and change the input/ to input/{model_name}/ (right now its \MUSE2\muse2_results\simple\input)
• remove the readonly implementation

was there anything else

Sounds good!

[tsmbland]

When you run just regenerate_test_data now, you get all the input folders along with the test data. I think this is fine, but we don't want to commit these files, so we should add **/input/* to the tests/data/.gitignore file.

@alexdewar I had the same comment but a different suggestion! I think this is a good idea as well/instead. I still think it might be worth making it configurable though - what do you think?

I'm not averse to making it configurable. I guess for consistency with the other options though, we should make it configurable in settings.toml as well as via a command-line flag. Do you agree @tsmbland?

Yes I would say so! @mjademitchell Happy if you'd rather open this as an issue to tackle in another PR. Not necessarily essential for this PR

[tsmbland]

...and on that note, I'm wondering if it might be nicer if we could come up with a way that users can override arbitrary options for settings.toml with command-line flags without having all this duplicated code. Maybe Copilot has some ideas...

@alexdewar I think that could work, so long as we don't end up making settings.toml more complicated in the long run like adding tables/arrays. I don't really see that happening though. Worth opening an issue about this?

Edit: Also that some options in settings.toml don't apply to all commands (e.g. graph_results_root doesn't apply to the run command, so it would be weird to allow it as a command-line arg even if it did nothing)

[alexdewar]

I'm not averse to making it configurable. I guess for consistency with the other options though, we should make it configurable in settings.toml as well as via a command-line flag. Do you agree @tsmbland?

Yes I would say so! @mjademitchell Happy if you'd rather open this as an issue to tackle in another PR. Not necessarily essential for this PR

Agree with this ☝️. Dw about fixing it now if it turns out to be a faff @mjademitchell!

...and on that note, I'm wondering if it might be nicer if we could come up with a way that users can override arbitrary options for settings.toml with command-line flags without having all this duplicated code. Maybe Copilot has some ideas...

@alexdewar I think that could work, so long as we don't end up making settings.toml more complicated in the long run like adding tables/arrays. I don't really see that happening though. Worth opening an issue about this?

Edit: Also that some options in settings.toml don't apply to all commands (e.g. graph_results_root doesn't apply to the run command, so it would be weird to allow it as a command-line arg even if it did nothing)

I've opened an issue for it (#1284). We might be able to bundle some options (e.g. the run-specific ones) into a separate struct, which would at least remove some of the deduplication. This will all inevitably be more fiddly than it would with Python though 😄

[mjademitchell]

@mjademitchell Side point, it would be worth running pre-commit install on your machine so it starts running the pre-commit hooks locally. Just saves the bot from having to do it on github

I am getting a bug with my pre commit for some reason its not working but maybe its my python verison

[alexdewar]

@mjademitchell Side point, it would be worth running pre-commit install on your machine so it starts running the pre-commit hooks locally. Just saves the bot from having to do it on github

I am getting a bug with my pre commit for some reason its not working but maybe its my python verison

What's the error message you see?

[alexdewar]

Looks like you've got some unresolved merge conflicts in output.rs. Could that be the culprit?

[mjademitchell]

@mjademitchell Side point, it would be worth running pre-commit install on your machine so it starts running the pre-commit hooks locally. Just saves the bot from having to do it on github

I am getting a bug with my pre commit for some reason its not working but maybe its my python verison

What's the error message you see?

[INFO] Restored changes from /home/mmitchel/.cache/pre-commit/patch1778745740-151170.
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/python3', '-mnodeenv', '--prebuilt', '--clean-src', '/home/mmitchel/.cache/pre-commit/repoy3fwau5k/node_env-default')
return code: 1
stdout: (none)
stderr:
/usr/lib/python3/dist-packages/nodeenv.py:24: DeprecationWarning: 'pipes' is deprecated and slated for removal in Python 3.13
import pipes
/usr/lib/python3/dist-packages/nodeenv.py:37: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
from pkg_resources import parse_version
Traceback (most recent call last):
File "", line 198, in _run_module_as_main
File "", line 88, in _run_code
File "/usr/lib/python3/dist-packages/nodeenv.py", line 1042, in
main()
File "/usr/lib/python3/dist-packages/nodeenv.py", line 881, in main
opt.node = get_last_stable_node_version()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/nodeenv.py", line 835, in get_last_stable_node_version
return links[-1][0]
~~~~~^^^^
IndexError: list index out of range
Check the log at /home/mmitchel/.cache/pre-commit/pre-commit.log

[alexdewar]

@mjademitchell Oh, that is weird. Looks like you're using Python 3.13, which is pretty up to date, so that should be fine. Could you try installing uv, then running uvx pre-commit install instead?

[mjademitchell]

Looks like you've got some unresolved merge conflicts in output.rs. Could that be the culprit?

thats weird i fixed this!

[alexdewar]

Oh, wait. I misread that. I don't know that you are using Python 3.13, actually. But anyway, might be worth trying with uv or reinstalling pre-commit with a newer version of Python.

Failing that, there is also prek, which should be a drop-in replacement for pre-commit. It's written in Rust, so you can do cargo install --locked prek, then prek install -f.

[mjademitchell]

Oh, wait. I misread that. I don't know that you are using Python 3.13, actually. But anyway, might be worth trying with uv or reinstalling pre-commit with a newer version of Python.

Failing that, there is also prek, which should be a drop-in replacement for pre-commit. It's written in Rust, so you can do cargo install --locked prek, then prek install -f.

It worked!

[alexdewar]

I've suggested a few small tweaks, but otherwise I think this is good to go!

Could you open an issue about adding a config and CLI option for turning the copying on/off?

[mjademitchell]

I've suggested a few small tweaks, but otherwise I think this is good to go!

Could you open an issue about adding a config and CLI option for turning the copying on/off?

Thats done! hopefully i wrote a good enough description

#1298