Skip to content
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

Add iteration time test as part of regression testing #358

Merged
merged 6 commits into from
Aug 7, 2023

Conversation

karan6181
Copy link
Collaborator

Description of changes:

  • Add iteration time test as part of regression testing

Issue #, if available:

STR-89

Merge Checklist:

Put an x without space in the boxes that apply. If you are unsure about any checklist, please don't hesitate to ask. We are here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the contributor guidelines
  • This is a documentation change or typo fix. If so, skip the rest of this checklist.
  • I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the MosaicML team.
  • I have updated any necessary documentation, including README and API docs (if appropriate).

Tests

  • I ran pre-commit on my change. (check out the pre-commit section of prerequisites)
  • I have added tests that prove my fix is effective or that my feature works (if appropriate).
  • I ran the tests locally to make sure it pass. (check out testing)
  • I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes.

@karan6181 karan6181 requested a review from b-chu August 2, 2023 06:38
Copy link
Contributor

@b-chu b-chu left a comment

Choose a reason for hiding this comment

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

It seems like the PR includes a refactor and a new regression test. Can you write a description of what you're refactoring (longer) and what iteration time is testing (shorter)?

Also would it be better to separate out this PR into 3 PRs? One for refactor, one for the new datasets, and one for the time test?

regression/iterate_data.py Show resolved Hide resolved
regression/iterate_data.py Outdated Show resolved Hide resolved
regression/utils.py Show resolved Hide resolved
regression/utils.py Outdated Show resolved Hide resolved
regression/synthetic_dataset.py Outdated Show resolved Hide resolved
regression/synthetic_dataset.py Outdated Show resolved Hide resolved
regression/iterate_data.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@b-chu b-chu left a comment

Choose a reason for hiding this comment

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

Was there something that changed the dataloader to return dictionaries of lists in a batch instead of dictionaries of tensors?

regression/synthetic_dataset.py Outdated Show resolved Hide resolved
@karan6181 karan6181 marked this pull request as ready for review August 7, 2023 15:42
@karan6181 karan6181 merged commit 7935a9d into mosaicml:main Aug 7, 2023
5 checks passed
@karan6181 karan6181 deleted the iter_time_reg_test branch August 7, 2023 15:54
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