Ignore `step` param in Neptune logger's log_metric method #5510

PiotrJander · 2021-01-14T09:55:06Z

What does this PR do?

The step parameter is ignored in log_metrics() because Neptune requires strictly increasing step values, a condition which is sometimes violated in Lighting e.g. when fit() and test() are called one after another on some models. step could be enabled again once Lightning guarantees that step values are always strictly increasing.

Also a minor bugfix: the log_text() method should use Neptune's log_text() method.

Fixes #5130

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 🙃

The `step` parameter is ignored because Neptune requires strictly increasing step values, a condition which is sometimes violated in Lighting e.g. when `fit()` and `test()` are called one after another on some models. `step` could be enabled again once Lightning guarantees that step values are always strictly increasing. Also a minor bugfix: the `log_text()` method should use Neptune's `log_text()` method.

awaelchli · 2021-01-14T14:18:50Z

pytorch_lightning/loggers/neptune.py

+            # `step` is ignored because Neptune expects strictly increasing step values which
+            # Lighting does not always guarantee.
+            self.log_metric(key, val)


It normally is, unless user adds new logs manually then the neptune's internal step may increase while lightning's step does not.

it's probably the same as in WandB logger, see #5194
if more flexibility is needed in the future, we could consider something similar as proposed in #5351

Note: the global step in Lightning should not increase during val or test. It is not a bug.

Thanks for this comment - could you just have a look at this related issue (and the specific linked comment) and confirm that this behaviour of PTL is expected? #5130 (comment)

In reply to the comment in the link:
Multiple calls to fit will not reset the global step. However, if the user creates a new trainer after fit and then calls fit again on the new trainer, this one will start from global step 0 (but then this would probably also create a new neptune experiment)

Using global step for logging is one possibility. I understand that some loggers may want to resume and append new data points with a monotonic step. I think that's perfectly fine. If the usability of Neptune benefits from tracking an internal step separate from Trainers global step, then that should be fine in my opinion. However, the user should be careful, because if they do multiple log calls per actual training step, they may run into issues comparing experiments across runs because the step axes don't match up.

awaelchli · 2021-01-14T14:23:35Z

Let's treat this as an api change? or do you see this as a bugfix? Is there a related issue that we need to link?

PiotrJander · 2021-01-14T20:16:02Z

I suppose it could be viewed as the latter (a bugfix). The API stays the same in terms of function signatures, and the behavior changes in a very minor way - I don't think many users rely on steps numbering for anything critical (but I might be mistaken of course).

…

On Thu, Jan 14, 2021 at 3:23 PM Adrian Wälchli ***@***.***> wrote: Let's treat this as an api change? or do you see this as a bugfix? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5510 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACK76PQDAUYWWITT2GC2YZDSZ347PANCNFSM4WCFLIHA> .

awaelchli · 2021-01-15T02:20:55Z

What's left to do is you need to adjust the two tests:
test_neptune_additional_methods and test_logger_with_prefix_all in the files tests/loggers/test_neptune.py and test/loggers/test_all.py
Let me know if you need help with that

PiotrJander · 2021-01-18T21:21:26Z

I did fix (or tried to fix) the test you mentioned but the CI reports shows many unrelated tests failing, and I'm not sure I can determine whether all relevant tests are passing now. But if they do, then please let me know and I'll merge this.

awaelchli · 2021-01-20T03:42:41Z

@PiotrJander I fixed the tests. Have a look. The remaining failures do not originate from this PR.

codecov · 2021-01-20T04:06:56Z

Codecov Report

Merging #5510 (a7e9a0d) into master (6ab5417) will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5510   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         135     135           
  Lines       10005   10005           
======================================
  Hits         9339    9339           
  Misses        666     666

* Ignore `step` param in Neptune logger's log_metric method The `step` parameter is ignored because Neptune requires strictly increasing step values, a condition which is sometimes violated in Lighting e.g. when `fit()` and `test()` are called one after another on some models. `step` could be enabled again once Lightning guarantees that step values are always strictly increasing. Also a minor bugfix: the `log_text()` method should use Neptune's `log_text()` method. * Update neptune.py * Update test_neptune.py * Update test_all.py * fix neptune tests * add chlog Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Nicki Skafte <[email protected]> (cherry picked from commit 5d76b31)

PiotrJander requested review from awaelchli, Borda, justusschock, SeanNaren, tchaton and williamFalcon as code owners January 14, 2021 09:55

Update neptune.py

5844186

awaelchli approved these changes Jan 14, 2021

View reviewed changes

awaelchli added the logger Related to the Loggers label Jan 14, 2021

awaelchli added this to the 1.2 milestone Jan 14, 2021

pitercl mentioned this pull request Jan 18, 2021

NeptuneObserver raises Neptune.api_exceptions.ChannelsValuesSendBatchError #5130

Closed

PiotrJander added 2 commits January 18, 2021 21:51

Update test_neptune.py

08bfb65

Update test_all.py

3f3c061

awaelchli added 3 commits January 20, 2021 04:24

fix neptune tests

6f5595f

add chlog

3e695f7

Merge branch 'master' into neptune-fix

55a2549

awaelchli added bug Something isn't working ready PRs ready to be merged labels Jan 20, 2021

mergify bot requested a review from a team January 21, 2021 16:13

mergify bot added 2 commits January 22, 2021 10:05

Merge branch 'master' into patch-1

a7e9a0d

Merge branch 'master' into patch-1

a812a4a

tchaton approved these changes Jan 24, 2021

View reviewed changes

mergify bot added the has conflicts label Jan 24, 2021

Merge branch 'master' into patch-1

e60be4a

SkafteNicki approved these changes Jan 25, 2021

View reviewed changes

SkafteNicki enabled auto-merge (squash) January 25, 2021 08:33

mergify bot removed the has conflicts label Jan 25, 2021

mergify bot added 2 commits January 25, 2021 09:39

Merge branch 'master' into patch-1

ca25683

Merge branch 'master' into patch-1

1bd1cc2

SkafteNicki merged commit 5d76b31 into Lightning-AI:master Jan 25, 2021

PiotrJander deleted the patch-1 branch January 25, 2021 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore `step` param in Neptune logger's log_metric method #5510

Ignore `step` param in Neptune logger's log_metric method #5510

PiotrJander commented Jan 14, 2021 •

edited by awaelchli

Loading

awaelchli Jan 14, 2021 •

edited

Loading

PiotrJander Jan 18, 2021

awaelchli Jan 20, 2021 •

edited

Loading

awaelchli commented Jan 14, 2021 •

edited

Loading

PiotrJander commented Jan 14, 2021 via email

awaelchli commented Jan 15, 2021 •

edited

Loading

PiotrJander commented Jan 18, 2021

awaelchli commented Jan 20, 2021

codecov bot commented Jan 20, 2021 •

edited

Loading

Ignore step param in Neptune logger's log_metric method #5510

Ignore step param in Neptune logger's log_metric method #5510

Conversation

PiotrJander commented Jan 14, 2021 • edited by awaelchli Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

awaelchli Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

PiotrJander Jan 18, 2021

Choose a reason for hiding this comment

awaelchli Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

awaelchli commented Jan 14, 2021 • edited Loading

PiotrJander commented Jan 14, 2021 via email

awaelchli commented Jan 15, 2021 • edited Loading

PiotrJander commented Jan 18, 2021

awaelchli commented Jan 20, 2021

codecov bot commented Jan 20, 2021 • edited Loading

Codecov Report

Ignore `step` param in Neptune logger's log_metric method #5510

Ignore `step` param in Neptune logger's log_metric method #5510

PiotrJander commented Jan 14, 2021 •

edited by awaelchli

Loading

awaelchli Jan 14, 2021 •

edited

Loading

awaelchli Jan 20, 2021 •

edited

Loading

awaelchli commented Jan 14, 2021 •

edited

Loading

awaelchli commented Jan 15, 2021 •

edited

Loading

codecov bot commented Jan 20, 2021 •

edited

Loading