Skip to content

Move settings.toml out of model dir#637

Merged
alexdewar merged 6 commits intomainfrom
move-settings.toml-from-model-dir
Jun 23, 2025
Merged

Move settings.toml out of model dir#637
alexdewar merged 6 commits intomainfrom
move-settings.toml-from-model-dir

Conversation

@alexdewar
Copy link
Copy Markdown
Collaborator

@alexdewar alexdewar commented Jun 19, 2025

Description

I've been looking at the agent investment stuff and it's making my brain hurt, so I thought I'd change tack and do this issue that's been bothering me for a while. We currently keep settings.toml with model configuration files, but really it's for program settings (cf. model.toml which contains actual parameters), so it should live somewhere else. For now, I've just changed things so that muse2 looks for settings.toml in the CWD because that was the simplest option, though we could do something cleverer at some point if we want (#255).

I also thought it makes sense to bundle an example settings.toml file with the releases (although muse2 will also run fine if it's not present).

Running some of the tests now have to change the CWD, so I'm using this external crate to do this safely.

Closes #513.

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

@alexdewar alexdewar requested review from Copilot and tsmbland June 19, 2025 11:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR relocates settings.toml loading to the current working directory, updates tests accordingly, and includes an example settings file in release artifacts.

  • Switch from Settings::from_path(model_dir) to Settings::load(), which looks in the CWD
  • Adjust tests to manipulate CWD using the current_dir crate
  • Update CLI entrypoint and release workflow to include assets/settings.toml

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

File Description
src/settings.rs Replaced from_path with load; adjusted internal logic
src/commands.rs Updated command handler to call Settings::load()
Cargo.toml Added current_dir to dev-dependencies
.github/workflows/release.yml Copy assets/settings.toml into the release artifact bundle
Comments suppressed due to low confidence (3)

src/settings.rs:30

  • The doc comment still refers to from_path and model_dir. Update it to describe Settings::load() and its behavior (loading from the CWD or returning default).
    /// The program settings as a `Settings` struct or an error if the file is invalid

src/settings.rs:50

  • [nitpick] The test name still references from_path; consider renaming to test_settings_load_no_file to match the new load method.
    fn test_settings_from_path_no_file() {

src/settings.rs:58

  • [nitpick] Update this test name to test_settings_load so it aligns with the updated Settings::load() method.
    fn test_settings_from_path() {

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 19, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.47%. Comparing base (7d69c7d) to head (183350d).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/commands.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #637   +/-   ##
=======================================
  Coverage   88.46%   88.47%           
=======================================
  Files          39       39           
  Lines        3555     3558    +3     
  Branches     3555     3558    +3     
=======================================
+ Hits         3145     3148    +3     
  Misses        219      219           
  Partials      191      191           

☔ 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
Copy link
Copy Markdown
Collaborator

Looks reasonable I think. I'm still not super convinced that we actually need settings.toml and can't just do everything via command-line options, but if we end up adding loads of settings I may change my mind

@alexdewar alexdewar enabled auto-merge June 23, 2025 16:39
@alexdewar
Copy link
Copy Markdown
Collaborator Author

Looks reasonable I think. I'm still not super convinced that we actually need settings.toml and can't just do everything via command-line options, but if we end up adding loads of settings I may change my mind

We could get rid of it. But whatever we decide to do with it, I'd like for user settings not to live with model data 😛

@alexdewar alexdewar merged commit 6ab9ed2 into main Jun 23, 2025
7 checks passed
@alexdewar alexdewar deleted the move-settings.toml-from-model-dir branch June 23, 2025 16:41
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.

Move program settings out of model directory

3 participants