rename fast_dev_run -> unit_test #1087

Gokkulnath · 2020-03-07T17:26:07Z

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? Yes
Did you make sure to update the docs? Yes
Did you write any new necessary tests? Not Applicable
If you made a notable change (that affects users), did you update the CHANGELOG? I can update if required. It is a minor change.

What does this PR do?

PR review

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 🙃

pep8speaks · 2020-03-07T17:26:12Z

Hello @Gokkulnath! Thanks for updating this PR.

In the file pytorch_lightning/trainer/trainer.py:

Line 26:111: E501 line too long (115 > 110 characters)

Comment last updated at 2020-03-24 19:12:00 UTC

Borda · 2020-03-11T20:43:21Z

hey there, we have added GPU CI test, so could we kindly ask to rebase/merge master which will trigger these tests so we do not need to test it manually... Thx for your understanding 🤖

Borda

Thx for taking care about this change, here are some todos to be completed:

add deprecation warning
add backcompatibility - https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/trainer/deprecated_api.py
add tests - https://github.com/PyTorchLightning/pytorch-lightning/blob/master/tests/test_deprecated.py
update changelog

pytorch_lightning/trainer/__init__.py

Borda · 2020-03-19T21:49:56Z

@Gokkulnath mind rebase and check the comments above? :]

Gokkulnath · 2020-03-21T00:24:06Z

@Borda I am not sure why the CI testing is failing for ubuntu and MacOS can you please help me in resolving the issue ? Seems to work fine on Windows CI

williamFalcon · 2020-03-21T01:44:37Z

@Gokkulnath the rename is good. But this PR also needs to add a run through a test batch to be complete.

docs/source/debugging.rst

tests/test_deprecated.py

codecov · 2020-03-21T03:42:46Z

Codecov Report

Merging #1087 into master will increase coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff           @@
##           master   #1087   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          61      61           
  Lines        3153    3161    +8     
======================================
+ Hits         2880    2891   +11     
+ Misses        273     270    -3

Gokkulnath · 2020-03-22T15:49:14Z

@Borda I think the PR is ready for review. Kindly let me know if any more changes are required.

Borda

almost there :]

pytorch_lightning/trainer/evaluation_loop.py

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/training_loop.py

tests/test_deprecated.py

docs/source/debugging.rst

Borda

LGTM 🚀

Borda · 2020-03-25T00:03:20Z

FYI, just opens an issue on Trains clearml/clearml#119

tullie

The PR looks good to me so going to accept. I really do wonder if there's a better name for this flag though. "unit_test" is a fairly overloaded term in software engineering and imo it is slightly different to what this is. I'd personally prefer "batch_test_run" or similar. @williamFalcon how sold are you on the naming?

williamFalcon · 2020-03-25T16:30:08Z

not sold at all. just wanted to have something that people “get” from looking at the name. but agree that unit_test is overloaded. @ethanjperez

Borda · 2020-03-25T16:47:02Z

yeah, the Unit-test from sys eng. makes it a bit confusing... so what about dry_run?
@tullie @williamFalcon ^^

ethanjperez · 2020-03-25T18:34:40Z

dry-run sounds good, but makes it sound to me like you won't save anything (like checkpoints), like with rsync's dry-run. Maybe --debug-run?

Borda · 2020-03-25T18:55:13Z

dry-run sounds good, but makes it sound to me like you won't save anything (like checkpoints), like with rsync's dry-run. Maybe --debug-run?

so what about simple-run?

jeremyjordan · 2020-03-26T00:25:56Z

in my opinion, fast_dev_run is more informative - what's the motivation to change this?

ethanwharris · 2020-03-26T20:42:22Z

I'm with @jeremyjordan and @tullie on this. Unit_test to me implies testing some small unit of code rather than quickly leak testing the whole pipeline. Can't think of a good suggestion for a better name though ...

Borda · 2020-03-26T20:54:33Z

I'm with @jeremyjordan and @tullie on this. Unit_test to me implies testing some small unit of code rather than quickly leak testing the whole pipeline. Can't think of a good suggestion for a better name though ...

do we have any third option as unit_test is soft.develop and fast_dev_run is not very descriptive

Gokkulnath · 2020-03-26T21:03:43Z

What about quick_test_pipelines? Inspired from quickly leak testing the whole pipeline.

tullie · 2020-03-26T21:09:19Z

My favorite suggestion is probably debug-run but not totally convinced. What about “single_batch_run”?

jeremyjordan · 2020-03-27T02:02:40Z

there's a cost incurred when we change argument names, so i'd suggest we only do this when there's a clear improvement. what's not specific about fast_dev_run? in other words, what are we failing to communicate in that name? to me it's pretty clear, "oh we just want to do a quick run during development to make sure everything works"

mergify · 2020-03-27T02:08:17Z

This pull request is now in conflict... :(

Borda · 2020-03-27T07:14:32Z

there's a cost incurred when we change argument names, so i'd suggest we only do this when there's a clear improvement. what's not specific about fast_dev_run? in other words, what are we failing to communicate in that name? to me it's pretty clear, "oh we just want to do a quick run during development to make sure everything works"

very much agree with @jeremyjordan, let's hold this rename until we all agree on that the new name is better than the actual one... @Gokkulnath sorry for the confusion and very much appreciate your initiative, stay with us!

Gokkulnath · 2020-03-27T12:20:25Z

@Borda No problem 👍

Borda · 2020-03-30T16:16:14Z

@PyTorchLightning/core-contributors are continuing in this renaming

williamFalcon · 2020-03-30T22:40:24Z

@Gokkulnath thanks for the contribution! this is my fault for not having arrived at a better name early haha. Please continute to contribute :)

rename fast_dev_run -> unit_test

aaeee45

Borda added this to the 0.7.2 milestone Mar 7, 2020

Borda added the feature Is an improvement or enhancement label Mar 7, 2020

Merge remote-tracking branch 'upstream/master'

df905df

Borda requested changes Mar 12, 2020

View reviewed changes

pytorch_lightning/trainer/__init__.py Outdated Show resolved Hide resolved

Update pytorch_lightning/trainer/__init__.py

cc1295c

Gokkulnath added 5 commits March 21, 2020 05:14

Merge remote-tracking branch 'upstream/master'

6d43309

Merge remote-tracking branch 'upstream/master'

49c047c

Fix conflicts

8fdc4ad

Remove argparse util not present in master

8d09b68

Fix Formatting flake8

6e284ca

Gokkulnath added 2 commits March 21, 2020 06:17

Fix deprecated api

bf68554

Fix failing test

32c36d8

awaelchli suggested changes Mar 21, 2020

View reviewed changes

docs/source/debugging.rst Show resolved Hide resolved

awaelchli reviewed Mar 21, 2020

View reviewed changes

tests/test_deprecated.py Outdated Show resolved Hide resolved

Fix Docs and test

84b94d7

Include test set

00067ea

Borda requested changes Mar 22, 2020

View reviewed changes

Make Requested Changes and Update test

7ccbc93

awaelchli reviewed Mar 22, 2020

View reviewed changes

docs/source/debugging.rst Outdated Show resolved Hide resolved

Gokkulnath added 3 commits March 23, 2020 04:29

Update Missing Docs

c687016

Update Missing Docs

117df82

Improve log message

9208af6

Borda approved these changes Mar 24, 2020

View reviewed changes

Borda added the ready PRs ready to be merged label Mar 25, 2020

Borda requested review from ethanwharris, jeremyjordan and a team March 25, 2020 14:38

tullie approved these changes Mar 25, 2020

View reviewed changes

Borda added discussion In a discussion stage and removed ready PRs ready to be merged labels Mar 26, 2020

Borda self-requested a review March 27, 2020 07:15

Borda mentioned this pull request Mar 27, 2020

Model Verification in Trainer #1237

Open

mergify bot requested a review from a team March 30, 2020 22:33

williamFalcon closed this Mar 30, 2020

Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename fast_dev_run -> unit_test #1087

rename fast_dev_run -> unit_test #1087

Gokkulnath commented Mar 7, 2020 •

edited

Loading

pep8speaks commented Mar 7, 2020 •

edited

Loading

Borda commented Mar 11, 2020

Borda left a comment •

edited

Loading

Borda commented Mar 19, 2020

Gokkulnath commented Mar 21, 2020 •

edited

Loading

williamFalcon commented Mar 21, 2020

codecov bot commented Mar 21, 2020 •

edited

Loading

Gokkulnath commented Mar 22, 2020

Borda left a comment

Borda left a comment

Borda commented Mar 25, 2020 •

edited

Loading

tullie left a comment

williamFalcon commented Mar 25, 2020 •

edited

Loading

Borda commented Mar 25, 2020 •

edited

Loading

ethanjperez commented Mar 25, 2020

Borda commented Mar 25, 2020

jeremyjordan commented Mar 26, 2020

ethanwharris commented Mar 26, 2020

Borda commented Mar 26, 2020

Gokkulnath commented Mar 26, 2020

tullie commented Mar 26, 2020

jeremyjordan commented Mar 27, 2020

mergify bot commented Mar 27, 2020

Borda commented Mar 27, 2020 •

edited

Loading

Gokkulnath commented Mar 27, 2020

Borda commented Mar 30, 2020

williamFalcon commented Mar 30, 2020

rename fast_dev_run -> unit_test #1087

rename fast_dev_run -> unit_test #1087

Conversation

Gokkulnath commented Mar 7, 2020 • edited Loading

Before submitting

What does this PR do?

PR review

Did you have fun?

pep8speaks commented Mar 7, 2020 • edited Loading

Comment last updated at 2020-03-24 19:12:00 UTC

Borda commented Mar 11, 2020

Borda left a comment • edited Loading

Choose a reason for hiding this comment

Borda commented Mar 19, 2020

Gokkulnath commented Mar 21, 2020 • edited Loading

williamFalcon commented Mar 21, 2020

codecov bot commented Mar 21, 2020 • edited Loading

Codecov Report

Gokkulnath commented Mar 22, 2020

Borda left a comment

Choose a reason for hiding this comment

Borda left a comment

Choose a reason for hiding this comment

Borda commented Mar 25, 2020 • edited Loading

tullie left a comment

Choose a reason for hiding this comment

williamFalcon commented Mar 25, 2020 • edited Loading

Borda commented Mar 25, 2020 • edited Loading

ethanjperez commented Mar 25, 2020

Borda commented Mar 25, 2020

jeremyjordan commented Mar 26, 2020

ethanwharris commented Mar 26, 2020

Borda commented Mar 26, 2020

Gokkulnath commented Mar 26, 2020

tullie commented Mar 26, 2020

jeremyjordan commented Mar 27, 2020

mergify bot commented Mar 27, 2020

Borda commented Mar 27, 2020 • edited Loading

Gokkulnath commented Mar 27, 2020

Borda commented Mar 30, 2020

williamFalcon commented Mar 30, 2020

Gokkulnath commented Mar 7, 2020 •

edited

Loading

pep8speaks commented Mar 7, 2020 •

edited

Loading

Borda left a comment •

edited

Loading

Gokkulnath commented Mar 21, 2020 •

edited

Loading

codecov bot commented Mar 21, 2020 •

edited

Loading

Borda commented Mar 25, 2020 •

edited

Loading

williamFalcon commented Mar 25, 2020 •

edited

Loading

Borda commented Mar 25, 2020 •

edited

Loading

Borda commented Mar 27, 2020 •

edited

Loading