-
Notifications
You must be signed in to change notification settings - Fork 37
regression test #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
regression test #234
Conversation
b58a3de
to
5e73cc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move these to a /test/* dir? It might get real annoying to grep through torchft/ if there's a bunch of testing specific autogenned files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I was thinking top level not under the torchft dir
I often find stuff by just running rg foo torchft
so was hoping to have it excluded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- moved to
/test_fixtures
- keep the test file with the implementation file
- renamed
test
folder to_test
"0": { | ||
"layers.0.weight": [ | ||
[ | ||
1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it expected that all of these are whole numbers? I would expect the weights to be a lot more varied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i'm optimizing for human readability e.g. i was able to run the numerics by hand and validate the output in these files myself. this helped in writing the validation logic and it caught issues that i fixed in the previous diff. also using floats might need us to do some closeness check because of precision issues (different machines might do things differently). i think we do get full validation of the logic using just whole numbers though?
9378ece
to
d5f0b32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary: - add a test that uses fixtures to validate against previous runs of the test - the fixutres can be written using `WRITE_FIXTURE=true pytest -vs ./torchft/test_diloco_mocked_updates.py` - when writing fixtures, the test also numerically validates the implementation of streaming diloco
Summary:
WRITE_FIXTURE=true pytest -vs ./torchft/test_diloco_mocked_updates.py