Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #646 +/- ##
==========================================
+ Coverage 62.95% 62.96% +0.01%
==========================================
Files 58 59 +1
Lines 5782 5730 -52
==========================================
- Hits 3640 3608 -32
+ Misses 2142 2122 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Click here to view all benchmarks. |
…that `batch["object_id"]` will be used for the ids.
- Move load_model_weights from config_utils.py to models/model_utils.py - Remove log_mlflow_params from config_utils.py - Restore _log_params method in test.py (no longer using shared function) - Revert all changes to train.py (back to original state) - Update imports in test.py and infer.py to use models.model_utils - Update tests to import from new location Co-authored-by: drewoldag <47493171+drewoldag@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated code from the test.py and infer.py verbs by extracting shared functionality into reusable utilities. The refactoring consolidates model weight loading and batch result saving logic, reducing code duplication and improving maintainability.
Changes:
- Extracted
load_model_weights()function tomodels/model_utils.pyfor shared model weight loading logic - Extracted
create_save_batch_callback()factory function topytorch_ignite.pyfor creating batch result saving callbacks - Updated
test.pyandinfer.pyto use the new shared utilities, removing ~180 lines of duplicated code - Updated test imports and function calls to reference the new utility functions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyrax/models/model_utils.py | New file containing load_model_weights() utility function that consolidates model weight loading logic from test and infer verbs |
| src/hyrax/pytorch_ignite.py | Added create_save_batch_callback() factory function that creates closures for saving batch results during inference/testing |
| src/hyrax/verbs/test.py | Removed load_model_weights() static method and _save_batch nested function, replaced with calls to new shared utilities |
| src/hyrax/verbs/infer.py | Removed load_model_weights() static method and _save_batch nested function, replaced with calls to new shared utilities |
| tests/hyrax/test_infer.py | Updated test imports to reference new load_model_weights() location in models/model_utils.py |
src/hyrax/pytorch_ignite.py
Outdated
| data_loader_indexes : array-like | ||
| Indices mapping data loader order to dataset order |
There was a problem hiding this comment.
The data_loader_indexes parameter is documented and accepted but never used in the function body. The old implementations in test.py and infer.py used this parameter as a fallback when batch didn't contain object_id. Consider either removing this parameter if it's truly unnecessary, or documenting why it's passed but unused.
…allback - Remove unused batch_len variable - Remove unused write_index variable and nonlocal reference - Add error handling to log and raise RuntimeError if batch doesn't contain object_id - Update docstring to remove mention of "indexes" parameter Co-authored-by: drewoldag <47493171+drewoldag@users.noreply.github.com>
Change Description
Two verb modules (
test.pyandinfer.py) contained significant code duplication for model weight loading and batch result saving.Solution Description
Extracted shared logic into reusable utilities:
load_model_weights()→models/model_utils.pyConsolidates ~96 lines of duplicated code from
test.pyandinfer.py. Handles explicit path or auto-detection from recent training run. Parameterized by verb name.create_save_batch_callback()→pytorch_ignite.pyConsolidates ~84 lines of duplicated code from
test.pyandinfer.py. Factory function returns closure that saves model outputs batch-by-batch during inference/testing. Includes validation and error handling to ensure batches contain requiredobject_idfield, with clear error messages to guide dataset implementation.Scope: Only
test.pyandinfer.pywere refactored. Thetrain.pyverb remains unchanged and retains its own_log_paramsmethod. Similarly,test.pyhas its own_log_paramsmethod (not shared with train).Impact: Reduced code duplication in test and infer verbs while maintaining clear separation of concerns between verbs. Removed unused variables (
batch_len,write_index) from the callback implementation for cleaner code.Breaking changes: None. All verb interfaces unchanged. The callback now requires datasets to provide
object_idin batch dictionaries, which was already the expected behavior in the original implementation.Code Quality
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.