Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Fix Video DDP #1189

Merged
merged 8 commits into from
Feb 23, 2022
Merged

Fix Video DDP #1189

merged 8 commits into from
Feb 23, 2022

Conversation

ethanwharris
Copy link
Collaborator

@ethanwharris ethanwharris commented Feb 23, 2022 •
edited
Loading

What does this PR do?

Fixes #1188

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@ethanwharris ethanwharris changed the title Bugfix/video ddp Fixing Video DDP Feb 23, 2022
@codecov
Copy link

codecov bot commented Feb 23, 2022 •
edited
Loading

Codecov Report

Merging #1189 (ca51db1) into master (5cf1321) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
+ Coverage   90.95%   90.96%   +0.01%     
==========================================
  Files         284      284              
  Lines       12687    12683       -4     
==========================================
- Hits        11539    11537       -2     
+ Misses       1148     1146       -2
Flag Coverage Δ
unittests 90.96% <0.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flash/core/trainer.py 92.22% <ø> (+1.01%) ⬆️
flash/video/classification/model.py 87.87% <0.00%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cf1321...ca51db1. Read the comment docs.

@ethanwharris ethanwharris added the bug / fix Something isn't working label Feb 23, 2022
@ethanwharris ethanwharris added this to the 0.7.x milestone Feb 23, 2022
@ethanwharris ethanwharris marked this pull request as ready for review February 23, 2022 11:21
@ethanwharris ethanwharris mentioned this pull request Feb 23, 2022
Closed
@ethanwharris ethanwharris changed the title Fixing Video DDP Fix Video DDP Feb 23, 2022
Borda
Borda approved these changes Feb 23, 2022
View reviewed changes
flash_examples/video_classification.py Show resolved Hide resolved
.azure-pipelines/gpu-example-tests.yml
@@ -14,6 +14,7 @@ jobs:
- "image"
- "text"
- "tabular"
- "video"
gpu_inds:
- "0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- "0"
- "1"

just the 0 may evoke no GPU...

krshrimali
krshrimali approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@krshrimali krshrimali left a comment

Choose a reason for hiding this comment

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

LGTM, just a small question - but shouldn't block merging this PR. 🎉

flash/core/trainer.py
@@ -79,7 +79,6 @@ def __init__(self, *args, **kwargs):
if flash._IS_TESTING:
if torch.cuda.is_available():
kwargs["gpus"] = -1
kwargs["max_epochs"] = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this removal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it was forcing 3 epochs in the test but video classification takes too long so was timing out. Now each example sets it's own max epochs and we just use that 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thanks for the explanation.

@ethanwharris ethanwharris merged commit de4e856 into master Feb 23, 2022
@ethanwharris ethanwharris deleted the bugfix/video_ddp branch February 23, 2022 14:09
ethanwharris added a commit that referenced this pull request Mar 1, 2022
@ethanwharris
Fix Video DDP (#1189)
02fe620
ethanwharris added a commit that referenced this pull request Mar 1, 2022
@ethanwharris
Fix Video DDP (#1189)
32f2e62
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krshrimali krshrimali krshrimali approved these changes

@Borda Borda Borda approved these changes

@tchaton tchaton Awaiting requested review from tchaton

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@justusschock justusschock Awaiting requested review from justusschock

@carmocca carmocca Awaiting requested review from carmocca

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

Assignees
No one assigned
Labels
bug / fix Something isn't working
Projects
None yet
Milestone
0.7.x
Development

Successfully merging this pull request may close these issues.

Multi-gpu training using flash fails for video classification
3 participants
@ethanwharris @Borda @krshrimali