advanced profiler describe + cleaned up tests #837

jeremyjordan · 2020-02-14T04:06:12Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
Did you read the contributor guideline?
Did you make sure to update the docs?
Did you write any new necessary tests?

What does this PR do?

Fixes #836

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.

Borda

is it covered by tests?

Borda · 2020-02-14T09:24:07Z

tests/test_profiler.py

@@ -26,6 +26,9 @@ def test_simple_profiler():
    np.testing.assert_allclose(p.recorded_durations["b"], [2], rtol=0.2)
    np.testing.assert_allclose(p.recorded_durations["c"], [1], rtol=0.2)

+    # ensure this doesn't throw any errors
+    description = p.describe()


I do not see description to be used...

yes i’m just making sure that it can successfully call the method without throwing an error. might be worth moving that line to a new test case test_simple_profiler_describe()

you may just drop output _ = p.describe() or just p.describe()
I would say that single letter variable is not good practice... :]

agreed, i'll clean the tests up a bit

Borda

Pls write a line about the philosophy behind using fixtures, it will be useful for future users...

Borda · 2020-02-15T23:01:07Z

tests/test_profiler.py

-        time.sleep(2)
+@pytest.mark.parametrize("action,expected", [("a", [3, 1]), ("b", [2]), ("c", [1])])
+def test_simple_profiler_durations(simple_profiler, action, expected):
+    """


just thinking if we shall start reasonable documentation also for tests, meaning at least add variable types...
if I would be perfectionist, doc requires Sentences start with a capital and ending by . :]

Borda · 2020-02-15T23:04:42Z

tests/test_profiler.py

+PROFILER_OVERHEAD_MAX_TOLERANCE = 0.0001
+
+
+@pytest.fixture


just to clarify, this freeze the profiler object so all test are using the same instance...

it depends on the scope of your fixture. by default, a new object is created for each function

https://docs.pytest.org/en/latest/reference.html#pytest-fixture

Borda · 2020-02-15T23:06:38Z

tests/test_profiler.py

+@pytest.mark.parametrize("action,expected", [("a", [3, 1]), ("b", [2]), ("c", [1])])
+def test_simple_profiler_durations(simple_profiler, action, expected):
+    """
+    ensure the reported durations are reasonably accurate


it took a mi sec to understand the idea behind, that is looping with the same instance... maybe it would be nice to write it in doc, in someone in future would need to change/update it...

i think it might be a good idea to update the test/README.md with testing philosophy and guidelines. introduce the usage of fixtures/parameterized tests, specify the desired level of documentation, etc.

what do you think?

good point, just not README but CONTRIBUTION on docs - in another issue/PR?
still, please write the two lines about the holding instance for multiple runs (i assume it is a bit special case...) Thx :]

Borda · 2020-02-15T23:07:07Z

tests/test_profiler.py

-def test_advanced_profiler():
-    def _get_duration(profile):
-        return sum([x.totaltime for x in profile.getstats()])
+def test_simple_profiler_overhead(simple_profiler, n_iter=5):


Borda · 2020-02-15T23:08:05Z

tests/test_profiler.py

-        time.sleep(2)
+@pytest.mark.parametrize("action,expected", [("a", [3, 1]), ("b", [2]), ("c", [1])])
+def test_advanced_profiler_durations(advanced_profiler, action, expected):
+    def _get_total_duration(profile):


_get_total_duration >> _sum_durations ?

i think total_duration is more explicit? as in, for a given action we're getting the total duration for all of the times the action was recorded. we're not actually summing over each invocation of the action, we're summing over the functions invoked within the action as recorded by the cProfiler

ok for total, just think that sum is a better fit than get...

Borda · 2020-02-15T23:10:33Z

tests/test_profiler.py

+            pass
+
+    action_profile = advanced_profiler.profiled_actions["no-op"]
+    total_duration = sum([x.totaltime for x in action_profile.getstats()])


if you declare previous _get_total_duration(profile) as module function you can just use it here...

good point :)

Borda · 2020-02-15T23:13:23Z

agreed, i'll clean the tests up a bit

@jeremyjordan really nice refactoring, just a minor comments 🚀

Borda · 2020-02-16T08:09:06Z

@jeremyjordan have made your last changes or @williamFalcon was faster with his merge?

jeremyjordan · 2020-02-16T15:52:37Z

@Borda the latter. i had planned to extract _get_total_duration as a module function and update the docs to be sentences

williamFalcon · 2020-02-16T15:54:00Z

@jeremyjordan sorry, i thought this was ready because there was no WIP. Can you open another PR to finish these changes? :)

jeremyjordan · 2020-02-16T16:04:00Z

@williamFalcon yeah no worries that was my bad! sure thing :)

* add py36 compatibility * add test case to capture previous bug * clean up tests * clean up tests

* add more underline * fix LightningMudule import error * remove unneeded blank line * escape asterisk to fix inline emphasis warning * add PULL_REQUEST_TEMPLATE.md * add __init__.py and import imagenet_example * fix duplicate label * add noindex option to fix duplicate object warnings * remove unexpected indent * refer explicit LightningModule * fix minor bug * refer EarlyStopping explicitly * restore exclude patterns * change the way how to refer class * remove unused import * update badges & drop Travis/Appveyor (#826) * drop Travis * drop Appveyor * update badges * fix missing PyPI images & CI badges (#853) * docs - anchor links (#848) * docs - add links * add desc. * add Greeting action (#843) * add Greeting action * Update greetings.yml Co-authored-by: William Falcon <[email protected]> * add pep8speaks (#842) * advanced profiler describe + cleaned up tests (#837) * add py36 compatibility * add test case to capture previous bug * clean up tests * clean up tests * Update lightning_module_template.py * Update lightning.py * respond lint issues * break long line * break more lines * checkout conflicting files from master * shorten url * checkout from upstream/master * remove trailing whitespaces * remove unused import LightningModule * fix sphinx bot warnings * Apply suggestions from code review just to trigger CI * Update .github/workflows/greetings.yml Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: William Falcon <[email protected]> Co-authored-by: Jeremy Jordan <[email protected]>

* add more underline * fix LightningMudule import error * remove unneeded blank line * escape asterisk to fix inline emphasis warning * add PULL_REQUEST_TEMPLATE.md * add __init__.py and import imagenet_example * fix duplicate label * add noindex option to fix duplicate object warnings * remove unexpected indent * refer explicit LightningModule * fix minor bug * refer EarlyStopping explicitly * restore exclude patterns * change the way how to refer class * remove unused import * update badges & drop Travis/Appveyor (Lightning-AI#826) * drop Travis * drop Appveyor * update badges * fix missing PyPI images & CI badges (Lightning-AI#853) * docs - anchor links (Lightning-AI#848) * docs - add links * add desc. * add Greeting action (Lightning-AI#843) * add Greeting action * Update greetings.yml Co-authored-by: William Falcon <[email protected]> * add pep8speaks (Lightning-AI#842) * advanced profiler describe + cleaned up tests (Lightning-AI#837) * add py36 compatibility * add test case to capture previous bug * clean up tests * clean up tests * Update lightning_module_template.py * Update lightning.py * respond lint issues * break long line * break more lines * checkout conflicting files from master * shorten url * checkout from upstream/master * remove trailing whitespaces * remove unused import LightningModule * fix sphinx bot warnings * Apply suggestions from code review just to trigger CI * Update .github/workflows/greetings.yml Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: William Falcon <[email protected]> Co-authored-by: Jeremy Jordan <[email protected]>

Jeremy Jordan added 2 commits February 13, 2020 23:01

add py36 compatibility

a5ebd63

add test case to capture previous bug

eedea96

Borda requested changes Feb 14, 2020

View reviewed changes

jeremyjordan changed the title ~~Bugfix/advanced profiler describe~~ WIP: bugfix/advanced profiler describe Feb 14, 2020

clean up tests

13de166

jeremyjordan changed the title ~~WIP: bugfix/advanced profiler describe~~ bugfix/advanced profiler describe + cleaned up tests Feb 15, 2020

Jeremy Jordan added 2 commits February 15, 2020 12:07

clean up tests

b90aa21

merge with origin

232ea49

jeremyjordan requested a review from Borda February 15, 2020 17:28

Borda changed the title ~~bugfix/advanced profiler describe + cleaned up tests~~ advanced profiler describe + cleaned up tests Feb 15, 2020

Borda added the bug Something isn't working label Feb 15, 2020

Borda added this to the 0.6.1 milestone Feb 15, 2020

Borda reviewed Feb 15, 2020

View reviewed changes

Borda added feature Is an improvement or enhancement need fix labels Feb 15, 2020

williamFalcon merged commit 4ae31cd into Lightning-AI:master Feb 16, 2020

jeremyjordan mentioned this pull request Feb 16, 2020

Add test guidance to contributer guide #862

Closed

jeremyjordan mentioned this pull request Feb 16, 2020

clean up tests/test_profiler.py #867

Merged

hanbyul-kim pushed a commit to hanbyul-kim/pytorch-lightning that referenced this pull request Feb 16, 2020

advanced profiler describe + cleaned up tests (Lightning-AI#837)

f9b548c

* add py36 compatibility * add test case to capture previous bug * clean up tests * clean up tests

hanbyul-kim pushed a commit to hanbyul-kim/pytorch-lightning that referenced this pull request Feb 24, 2020

advanced profiler describe + cleaned up tests (Lightning-AI#837)

28f6de2

* add py36 compatibility * add test case to capture previous bug * clean up tests * clean up tests

		PROFILER_OVERHEAD_MAX_TOLERANCE = 0.0001


		@pytest.fixture

advanced profiler describe + cleaned up tests #837

advanced profiler describe + cleaned up tests #837

Uh oh!

Conversation

jeremyjordan commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before submitting

What does this PR do?

PR review

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Borda commented Feb 15, 2020

Uh oh!

Borda commented Feb 16, 2020

Uh oh!

jeremyjordan commented Feb 16, 2020

Uh oh!

williamFalcon commented Feb 16, 2020

Uh oh!

jeremyjordan commented Feb 16, 2020

Uh oh!

Uh oh!

jeremyjordan commented Feb 14, 2020 •

edited

Loading