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

Create results folder and save log files to it #359

Merged
merged 7 commits into from
Jan 31, 2025
Merged

Create results folder and save log files to it #359

merged 7 commits into from
Jan 31, 2025

Conversation

alexdewar
Copy link
Collaborator

Description

We'll need somewhere to save the results for when we want to write the various output files, so I thought it would be worth writing the code to create the output folder now. It seemed a bit weird to just make an empty folder though, so I did #110 at the same time, so we at least have log files in there for now. The logging code has ended up a bit convoluted now (because we've chosen a complex design), but hopefully it's not too messy. Writing tests proved tricky so I didn't bother 😉. I've done manual testing of the various parts though.

Now we have:

  • Warnings and errors go to stderr and the muse2_error.log file
  • All other messages go to stdout and muse2_info.log file

The user can still configure the log level via settings.toml or an environment variable. However, this does not affect the log files, as I figured we want to capture everything in those. There is an exception: if the user sets the log level to be more verbose than the default (e.g. they set it to debug) then muse2_info.log will contain these extra messages too.

Closes #110.

Limitations

For now, if you run the same model twice, it'll use the same output folder for both and will append to the existing log files for the second run. This is probably fine for log files, but when we come to writing data we won't want to overwrite existing CSV files, so we'll want to do something else (error if the folder exists append a number/timestamp etc.).

The integration test we have (test_handle_run_command in command.rs) will now also write these log files to the output folder when we run it. This isn't ideal, because we don't know what state that output folder will be in (it may already exist with log files). It would be better to create a temporary directory to save the log files to, but the current code doesn't allow that. We could refactor things, but I didn't want to muck about in command.rs too much as @Sahil590 is also working on it.

I'll open issues for both of those things.

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

Further checks

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

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 38 lines in your changes missing coverage. Please review.

Project coverage is 95.22%. Comparing base (1230e4a) to head (6f3589c).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/log.rs 60.56% 21 Missing and 7 partials ⚠️
src/output.rs 55.55% 5 Missing and 3 partials ⚠️
src/commands.rs 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   95.15%   95.22%   +0.07%     
==========================================
  Files          30       31       +1     
  Lines        3548     3604      +56     
  Branches     3548     3604      +56     
==========================================
+ Hits         3376     3432      +56     
- Misses         90       92       +2     
+ Partials       82       80       -2     

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

@alexdewar
Copy link
Collaborator Author

PS -- Don't feel you have to scrutinise the logging-specific parts of this code particularly hard unless you want to! It basically just does what it did before except now also writes a couple of files too.

Copy link
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

Looks good for now, with the caveats you've mentioned

Other points/suggestions:

  • I think eventually we'll want to make the results path configurable, either via an option in the settings file or a command-line argument
  • MUSE1 has a replace parameter in the settings file to determine whether existing files in the results path should be overwritten, or an error raised. We could consider something similar

@alexdewar alexdewar enabled auto-merge January 31, 2025 15:12
@alexdewar alexdewar disabled auto-merge January 31, 2025 15:12
@alexdewar
Copy link
Collaborator Author

@tsmbland All good points!

I've opened various issues for these and other bits: #370 #371 #372 #373

@alexdewar alexdewar enabled auto-merge January 31, 2025 15:13
@alexdewar alexdewar disabled auto-merge January 31, 2025 15:14
@alexdewar alexdewar enabled auto-merge January 31, 2025 16:55
@alexdewar alexdewar merged commit 0d20127 into main Jan 31, 2025
7 checks passed
@alexdewar alexdewar deleted the log-to-file branch January 31, 2025 16:56
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.

Add options to the logger to log to a file
2 participants