Skip to content

[AUTO-MERGE] Removed unused input argument in create_results_writer#733

Merged
drewoldag merged 5 commits intomainfrom
issue/724/clean_create_results_writer
Mar 3, 2026
Merged

[AUTO-MERGE] Removed unused input argument in create_results_writer#733
drewoldag merged 5 commits intomainfrom
issue/724/clean_create_results_writer

Conversation

@drewoldag
Copy link
Copy Markdown
Collaborator

@drewoldag drewoldag commented Feb 27, 2026

There was an unused input parameter on create_results_writer, so I've removed that and chased down the various places it was used.

@drewoldag drewoldag self-assigned this Feb 27, 2026
@drewoldag drewoldag marked this pull request as ready for review February 27, 2026 21:07
Copilot AI review requested due to automatic review settings February 27, 2026 21:07
@drewoldag drewoldag linked an issue Feb 27, 2026 that may be closed by this pull request
@drewoldag drewoldag requested review from a team and mtauraso February 27, 2026 21:07
@drewoldag drewoldag requested review from a team and removed request for a team February 27, 2026 21:07
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 removes an unused dataset argument from create_results_writer (and the related create_save_batch_callback factory), and updates Hyrax verbs to call the simplified APIs when writing inference/test/UMAP/engine results.

Changes:

  • Update create_results_writer to accept only result_dir and remove the unused dataset parameter.
  • Update create_save_batch_callback to accept only results_dir and adjust its internal writer creation.
  • Update call sites in infer, test, umap, and engine verbs to use the new signatures.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/hyrax/verbs/umap.py Update results-writer creation to pass only results_dir.
src/hyrax/verbs/test.py Update save-batch callback creation to pass only results_dir.
src/hyrax/verbs/infer.py Update save-batch callback creation to pass only results_dir.
src/hyrax/verbs/engine.py Update results-writer creation to pass only result_dir.
src/hyrax/pytorch_ignite.py Remove dataset param from create_save_batch_callback and simplify writer creation.
src/hyrax/data_sets/result_factories.py Remove unused dataset param from create_results_writer and drop unused import.

Comment thread src/hyrax/data_sets/result_factories.py
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.16%. Comparing base (0096924) to head (d565a26).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/hyrax/verbs/engine.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
- Coverage   64.17%   64.16%   -0.01%     
==========================================
  Files          61       61              
  Lines        5990     5989       -1     
==========================================
- Hits         3844     3843       -1     
  Misses       2146     2146              

☔ 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.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 27, 2026

Before [0096924] After [d9dc89c] 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
1.5G 1.63G 1.09 vector_db_benchmarks.VectorDBInsertBenchmarks.peakmem_load_vector_db(16384, 'chromadb')
9.15±0.03ms 9.58±0.03ms 1.05 vector_db_benchmarks.VectorDBSearchBenchmarks.time_search_by_vector_many_shards(128, 'chromadb')
1.77±0.01s 1.80±0.02s 1.02 benchmarks.time_database_connection_help
1.76±0s 1.80±0.02s 1.02 benchmarks.time_umap_help
9.23±0.06ms 9.42±0.02ms 1.02 vector_db_benchmarks.VectorDBSearchBenchmarks.time_search_by_vector_many_shards(64, 'chromadb')
1.78±0.01s 1.79±0.02s 1.01 benchmarks.time_download_help
1.77±0.01s 1.78±0.02s 1.01 benchmarks.time_rebuild_manifest_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.

Looks good

@drewoldag drewoldag enabled auto-merge (squash) March 3, 2026 20:31
@drewoldag drewoldag changed the title Removed unused input argument in create_results_writer [AUTO-MERGE] Removed unused input argument in create_results_writer Mar 3, 2026
@drewoldag drewoldag merged commit 3d4429c into main Mar 3, 2026
7 checks passed
@drewoldag drewoldag deleted the issue/724/clean_create_results_writer branch March 3, 2026 21:31
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.

Clean up pytorch_ignite.py:create_save_batch_callback

3 participants