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

[tests/models] refactor with BoringModel #5507

Merged
merged 39 commits into from
Feb 11, 2021
Merged

[tests/models] refactor with BoringModel #5507

merged 39 commits into from
Feb 11, 2021

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented Jan 13, 2021 •
edited
Loading

What does this PR do?

Part of #4501

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • 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? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
  • Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

@rohitgr7 rohitgr7 changed the base branch from master to release/1.2-dev January 13, 2021 21:38
@Lightning-AI Lightning-AI deleted a comment from review-notebook-app bot Jan 13, 2021
@rohitgr7 rohitgr7 added refactor ci Continuous Integration labels Jan 16, 2021
@rohitgr7 rohitgr7 added this to the 1.2 milestone Jan 16, 2021
@rohitgr7 rohitgr7 marked this pull request as ready for review January 16, 2021 21:53
@rohitgr7
Copy link
Contributor Author

Need help with the 3 failing GPU tests.

@gianscarpe
Copy link
Contributor

Need help with the 3 failing GPU tests.

I can check and help you on that, as I worked on test_gpu and developed some of the structure

@rohitgr7
Copy link
Contributor Author

Need help with the 3 failing GPU tests.

I can check and help you on that, as I worked on test_gpu and developed some of the structure

that would be great. Thanks @gianscarpe

gianscarpe
gianscarpe reviewed Jan 20, 2021
View reviewed changes
tests/base/develop_pipelines.py Outdated Show resolved Hide resolved
tests/models/test_restore.py Outdated Show resolved Hide resolved
rohitgr7
rohitgr7 commented Jan 21, 2021
View reviewed changes
.drone.yml Outdated Show resolved Hide resolved
rohitgr7
rohitgr7 commented Jan 21, 2021
View reviewed changes
.drone.yml Outdated Show resolved Hide resolved
@gianscarpe
Copy link
Contributor

Hi @rohitgr7, great work with the refactoring of BoringModel. However, I believe it should be better to submit a smaller PR with less changes to help reviewers. @Borda what do you think?

@rohitgr7
Copy link
Contributor Author

@gianscarpe most of the changes here are independent so I guess the same amount of work will be required even if I split it into 2 or more. But yeah if others say to split this, it won't be a problem for me :)

Also, did you find anything regarding the remaining 2 failing GPU tests?

@Borda
Copy link
Member

Borda commented Jan 24, 2021 •
edited
Loading

Hi @rohitgr7, great work with the refactoring of BoringModel. However, I believe it should be better to submit a smaller PR with less changes to help reviewers. @Borda what do you think?

well yes, smaller PR would be easier to read, but since it changes just a few lines it shall be fine after all tests pass :]
@rohitgr7 mind mark all done comments as resolved? 🐰

@Borda Borda enabled auto-merge (squash) January 24, 2021 08:44
@Borda Borda requested a review from awaelchli January 24, 2021 08:44
@Borda Borda added the ready PRs ready to be merged label Jan 24, 2021
@rohitgr7 rohitgr7 removed the ready PRs ready to be merged label Jan 24, 2021
@rohitgr7 rohitgr7 force-pushed the tests/boring_models branch from 2f58059 to 4aa5a4b Compare February 10, 2021 13:41
@Borda
Copy link
Member

Borda commented Feb 10, 2021

@rohitgr7 @kaushikb11 mind check the last two failing tests? :]

awaelchli
awaelchli reviewed Feb 11, 2021
View reviewed changes
tests/core/test_datamodules.py
Comment on lines +393 to +394
def test_step_end(self, outputs):
self.log('test_acc', self.test_acc(outputs['logits'], outputs['y']))
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the meaning of this?
If we implement step_end we need to reduce outputs, right?
We get the outputs from all gpus.
for test accuracy, I recommend to compute it over the entire dataset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine, this is on step end not on epoch end, so the underlying metric will just accumulate via the update method. Only at epoch end will we call compute I think

This is to get around the issue that training step is within the DP scatter op, where the outputs will be on multiple devices

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I didn't see it's using a metric.
My concern was about the return None value here. But if that is handled internally then I guess no issue

Base automatically changed from release/1.2-dev to master February 11, 2021 14:32
@Borda Borda merged commit 8e9a026 into master Feb 11, 2021
@Borda Borda deleted the tests/boring_models branch February 11, 2021 14:32
@awaelchli
Copy link
Contributor

Why did you merge it, the tests were not passing

@Borda
Copy link
Member

Borda commented Feb 11, 2021

Why did you merge it, the tests were not passing

ok, that was an issue with switching branches, for while protection was off... :/

@awaelchli
Copy link
Contributor

@Borda what do you suggest? can we undo the commit and then resubmit the PR?

@Borda
Copy link
Member

Borda commented Feb 11, 2021 •
edited
Loading

@kaushikb11 is already working on that fix... I guess #5924

@awaelchli
Copy link
Contributor

Me and rohit were also working on the fix before it was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli left review comments

@Borda Borda Borda approved these changes

@akihironitta akihironitta akihironitta left review comments

@carmocca carmocca carmocca approved these changes

@gianscarpe gianscarpe gianscarpe approved these changes

@kaushikb11 kaushikb11 kaushikb11 approved these changes

@s-rog s-rog s-rog approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

Assignees

@gianscarpe gianscarpe

@kaushikb11 kaushikb11

Labels
ci Continuous Integration ready PRs ready to be merged refactor
Projects
None yet
Milestone
pl:1.2
Development

Successfully merging this pull request may close these issues.
10 participants
@rohitgr7 @gianscarpe @Borda @awaelchli @carmocca @pep8speaks @SeanNaren @akihironitta @kaushikb11 @s-rog