Skip to content

refactor: shared get_dataloader between train and evaluate#19

Merged
ziv-lazarov-nagish merged 3 commits intonagishfrom
refactor/shared-dataloader
Apr 16, 2026
Merged

refactor: shared get_dataloader between train and evaluate#19
ziv-lazarov-nagish merged 3 commits intonagishfrom
refactor/shared-dataloader

Conversation

@ziv-lazarov-nagish
Copy link
Copy Markdown
Contributor

@ziv-lazarov-nagish ziv-lazarov-nagish commented Apr 15, 2026

Summary

  • Move get_dataloader from train.py to datasets/common.py with explicit args parameter
  • evaluate.py uses shared get_dataloader instead of manual build_datasets + DataLoader
  • CI: run lint and test workflows on PRs targeting nagish branch

Changed files

  • datasets/common.py — add get_dataloader() with **augment_overrides support
  • train.py — remove local get_dataloader, import from common
  • evaluate.py — use get_dataloader instead of manual construction
  • datasets/__init__.py — export get_dataloader
  • .github/workflows/test.yaml / lint.yaml — add nagish to PR branch triggers

Test plan

  • ruff check . passes
  • pytest passes (61 tests)

…valuate

- Move get_dataloader from train.py to datasets/common.py with explicit
  args parameter (no global state dependency)
- evaluate.py uses shared get_dataloader instead of manual build_datasets + DataLoader
- Fix duplicate manifest in train.py (train + val collected same data)
@ziv-lazarov-nagish ziv-lazarov-nagish marked this pull request as ready for review April 15, 2026 13:22
Copy link
Copy Markdown
Contributor

@AmitMY AmitMY left a comment

Choose a reason for hiding this comment

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

actually, shouldn't some of it go into main?

@ziv-lazarov-nagish
Copy link
Copy Markdown
Contributor Author

actually, shouldn't some of it go into main?

it will, i was planning to cherry-pick the commits from nagish later instead of merging into main and then update nagish.

Copy link
Copy Markdown
Contributor

@AmitMY AmitMY left a comment

Choose a reason for hiding this comment

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

my approach would have been to update main, and then nagish
but you can also update nagish then ask claude to make a PR to main selecting only the non-spepcif nagish changes

@ziv-lazarov-nagish
Copy link
Copy Markdown
Contributor Author

my approach would have been to update main, and then nagish but you can also update nagish then ask claude to make a PR to main selecting only the non-spepcif nagish changes

that was my intention since i thought most of our efforts would be in the nagish branch going forward, so treating it as the "main" branch

@ziv-lazarov-nagish ziv-lazarov-nagish merged commit 5c2d74d into nagish Apr 16, 2026
2 checks passed
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.

2 participants