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.

Sign up for GitHub

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

Jump to bottom

Remove streaming v1 datasets. #1787

Merged
merged 17 commits into from
Dec 7, 2022
Merged

Remove streaming v1 datasets. #1787

merged 17 commits into from
Dec 7, 2022

Conversation

knighton
Copy link
Contributor

@knighton knighton commented Dec 6, 2022

No description provided.

@knighton knighton requested a review from karan6181 as a code owner December 6, 2022 09:41
@karan6181
Copy link
Contributor

Should we also remove files under composer/datasets/streaming folder ?

A-Jacobson
A-Jacobson approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@A-Jacobson A-Jacobson left a comment

Choose a reason for hiding this comment

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

signing off that the streaming COCO output is correct and the interface is consistent with the other datasets. Thanks James

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@karan6181 karan6181 requested a review from mvpatel2000 December 6, 2022 23:39
mvpatel2000
mvpatel2000 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

Do we want to keep the example notebooks with streaming v2?

@knighton
Copy link
Contributor Author

knighton commented Dec 7, 2022

Do we want to keep the example notebooks with streaming v2?

Was thinking of readding with fancy V3 afterward

karan6181
karan6181 approved these changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@karan6181 karan6181 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@knighton knighton merged commit 17b1671 into mosaicml:dev Dec 7, 2022
@knighton knighton deleted the james/unstream-v1 branch December 7, 2022 00:37
bmosaicml pushed a commit to bmosaicml/composer that referenced this pull request Dec 13, 2022
@knighton @bmosaicml
Remove streaming v1 datasets. (mosaicml#1787)
4426787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvpatel2000 mvpatel2000 mvpatel2000 approved these changes

@karan6181 karan6181 karan6181 approved these changes

@A-Jacobson A-Jacobson A-Jacobson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knighton @karan6181 @A-Jacobson @mvpatel2000