Skip to content

[AUTO-MERGE] Pass a deepcopy of config to verbs and Hyrax methods#757

Merged
drewoldag merged 3 commits intomainfrom
issue/703/no-overwriting-config
Mar 7, 2026
Merged

[AUTO-MERGE] Pass a deepcopy of config to verbs and Hyrax methods#757
drewoldag merged 3 commits intomainfrom
issue/703/no-overwriting-config

Conversation

@drewoldag
Copy link
Copy Markdown
Collaborator

Pass a deepcopy of the config to the various verbs and Hyrax methods that take a config as input. Added unit tests to check for config leakage and prevent regressions.

Change Description

Closes #703
This change will create a deep copy of the top level config before passing it to the verb (or quasi-verb that is called from a Hyrax method)

Perhaps we were trying to do this in the base Verb class with the self.config = config line, but that was allowing config changes to leak back up to the top level where they weren't appropriate.

This change allows user to pass in a config, and update the config interactively (i.e. in a notebook) but when a verb is called i.e. h.infer(), the updates to the config that the verb makes will not be seen by subsequent verb calls.

Issue #703 describes the scenario where this was first noticed.

…that take a config as input. Added unit tests to check for config leakage and prevent regressions.
@drewoldag drewoldag self-assigned this Mar 6, 2026
Copilot AI review requested due to automatic review settings March 6, 2026 18:16
@drewoldag
Copy link
Copy Markdown
Collaborator Author

Note, a larger change could involve cleaning up the config stuff throughout the various verbs. There is inconsistency about how the input configs are handled. In a later PR it might be nice to consolidate on one approach.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.69%. Comparing base (d4e6908) to head (666c490).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/hyrax/hyrax.py 45.45% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
- Coverage   64.70%   64.69%   -0.02%     
==========================================
  Files          61       61              
  Lines        5888     5894       +6     
==========================================
+ Hits         3810     3813       +3     
- Misses       2078     2081       +3     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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 aims to prevent Hyrax runtime config “leakage” by ensuring verbs (and verb-like Hyrax methods) receive an isolated deep-copied config, addressing the regression described in issue #703 where sequential train()/infer() cycles could reuse stale weights due to mutated config.

Changes:

  • Pass deepcopy(self.config) into verb dispatch (Hyrax.__getattr__) and into download/prepare/rebuild_manifest/raw_data_dimensions call paths.
  • Add regression tests ensuring sequential verb calls do not mutate h.config.
  • Add a unit test demonstrating that load_model_weights() mutations don’t affect the original config when a deepcopy is used.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/hyrax/hyrax.py Deep-copies self.config before dispatching to verbs and quasi-verbs to prevent mutation leaks into Hyrax.config.
tests/hyrax/test_infer.py Adds a unit test asserting config isolation when load_model_weights() is called with a deep-copied config.
tests/hyrax/test_config_isolation.py Adds regression tests to ensure train()/infer()/prepare() do not mutate the Hyrax instance’s config across calls.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread tests/hyrax/test_config_isolation.py Outdated
Comment thread src/hyrax/hyrax.py
Comment thread src/hyrax/hyrax.py
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 6, 2026

Before [d4e6908] After [0b88925] Ratio Benchmark (Parameter)
failed failed n/a data_cache_benchmarks.DataCacheBenchmarks.time_preload_cache_hsc1k
failed failed n/a data_cache_benchmarks.DataCacheBenchmarks.track_cache_hsc1k_hyrax_size_undercount
failed failed n/a data_request_benchmarks.DatasetRequestBenchmarks.time_request_all_data
40.4±0.4ms 41.0±2ms 1.02 benchmarks.time_nb_obj_dir
2.00±0.01s 2.03±0.01s 1.01 benchmarks.time_prepare_help
2.00±0.01s 2.01±0.01s 1.01 benchmarks.time_visualize_help
3.71G 3.75G 1.01 vector_db_benchmarks.VectorDBInsertBenchmarks.peakmem_load_vector_db(16384, 'qdrant')
285±4ms 285±2ms 1.00 benchmarks.time_import
2.01±0.01s 2.00±0s 1.00 benchmarks.time_infer_help
2.00±0.01s 2.01±0.02s 1.00 benchmarks.time_lookup_help

Click here to view all benchmarks.

Copy link
Copy Markdown
Collaborator

@mtauraso mtauraso left a comment

Choose a reason for hiding this comment

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

🚢 it

@drewoldag drewoldag enabled auto-merge (squash) March 7, 2026 00:39
@drewoldag drewoldag changed the title Pass a deepcopy of config to verbs and Hyrax methods [AUTO-MERGE] Pass a deepcopy of config to verbs and Hyrax methods Mar 7, 2026
@drewoldag drewoldag merged commit 618be04 into main Mar 7, 2026
6 of 7 checks passed
@drewoldag drewoldag deleted the issue/703/no-overwriting-config branch March 7, 2026 00:44
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.

Retraining a model uses the model weights from the previous training

3 participants