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.

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

Adapt NeptuneLogger to new neptune-client api #6867

Merged
merged 49 commits into from
Sep 10, 2021

Conversation

shnela
Copy link
Contributor

@shnela shnela commented Apr 7, 2021

Adapt NeptuneLogger to changes in neptune-client api changes

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

Did you have fun?

I made sure I had fun coding 🙃

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #6867 (9338bf3) into master (439db22) will decrease coverage by 3%.
The diff coverage is 91%.

@@           Coverage Diff           @@
##           master   #6867    +/-   ##
=======================================
- Coverage      92%     89%    -3%     
=======================================
  Files         179     180     +1     
  Lines       14904   15046   +142     
=======================================
- Hits        13750   13358   -392     
- Misses       1154    1688   +534     

@pep8speaks
Copy link

pep8speaks commented Apr 14, 2021

Hello @shnela! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-13 09:09:34 UTC

@awaelchli awaelchli added 3rd party Related to a 3rd-party logger Related to the Loggers feature Is an improvement or enhancement labels Apr 14, 2021
@shnela shnela force-pushed the dev/update_neptune_logger branch 2 times, most recently from 9c0bc40 to 4cfa2d0 Compare April 16, 2021 10:10
@shnela
Copy link
Contributor Author

shnela commented Apr 19, 2021

@williamFalcon @awaelchli @Borda @SeanNaren @carmocca @tchaton @justusschock @kaushikb11 @edenlightning

Hi guys.
Is there anything I have to do before you can start review of this branch?
From my perspective it's done, but if I need to fix something before you can start review, or if I can help you somehow with that, don't hesitate to ask ;)

.gitignore Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

LGTM on a high level.

README.md Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
@awaelchli awaelchli added this to the v1.4 milestone Apr 20, 2021
@Borda Borda added the design Includes a design discussion label Apr 20, 2021
Borda
Borda previously requested changes Apr 20, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Pls, consider rather use a new name for this new logger than rename the old Legacy...

CHANGELOG.md Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/neptune.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the has conflicts label Apr 22, 2021
@shnela
Copy link
Contributor Author

shnela commented Apr 22, 2021

@awaelchli @Borda

Ok, everything updated and ready to merge.

@awaelchli
Copy link
Contributor

Thanks a lot! We will follow up here as soon as 1.3 is released (May 5ish). We currently have feature freeze for stability testing and will not be able to merge new features and refactors.

@shnela shnela force-pushed the dev/update_neptune_logger branch from d9e6ca6 to 5b11de8 Compare September 9, 2021 18:15
@mergify mergify bot removed the has conflicts label Sep 9, 2021
@shnela
Copy link
Contributor Author

shnela commented Sep 10, 2021

@awaelchli @tchaton so can we merge now? :p

.github/CODEOWNERS Outdated Show resolved Hide resolved
@awaelchli awaelchli enabled auto-merge (squash) September 10, 2021 13:10
Co-authored-by: Adrian Wälchli <[email protected]>
auto-merge was automatically disabled September 10, 2021 13:24

Head branch was pushed to by a user without write access

@awaelchli awaelchli enabled auto-merge (squash) September 10, 2021 13:32
@carmocca carmocca merged commit ee37872 into Lightning-AI:master Sep 10, 2021
@tchaton
Copy link
Contributor

tchaton commented Sep 10, 2021

Hey @shnela,

Great work there ! Really excited about this :)

Best,
T.C

@shnela
Copy link
Contributor Author

shnela commented Sep 11, 2021

@tchaton

Great work there ! Really excited about this :)

We to! Thank you for review and your help!

Best,
Jakub

@aarzchan
Copy link

Hi! For the new NeptuneLogger, how do you set offline_mode? The only thing I could find in the docs is this.

@kamil-kaczmarek
Copy link
Contributor

Hi @aarzchan ,

In the new logger you can do it by parameterizing your Logger, like this (mode="offline"):

neptune_logger = NeptuneLogger(
    api_key="ANONYMOUS",  # replace with your own
    project="WORKSPACE/PROJECT",
    mode="offline",
)

There are more modes available.
Overall you pass any neptune.init() argument to the NeptuneLogger and in this way further define its' behavior.

@aarzchan
Copy link

Thanks, @kamil-kaczmarek!

@raman-r-4978
Copy link

Hi @shnela @kamil-kaczmarek ,
Is it possible to use old neptune API when using master? If yes, are there any examples?

@kamil-kaczmarek
Copy link
Contributor

Hi @raman-rajarathinam,

We are preparing an update to the neptune legacy-docs, where this use case will be supported and described.

We are planning to release an update next week.

@i008
Copy link

i008 commented Nov 5, 2021

Hi @kamil-kaczmarek after upgrading to pl==1.5.0 Neptune stopped working for me:

When i start it it print the experiment multiple times and keeps printing the messege as below.

https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
https://app.neptune.ai/i008/bovine/e/BOV-575
Global seed set to 1245
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:48.990Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:48.992Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:49.359Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:49.361Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:49.722Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:49.726Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:50.099Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:50.105Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:50.477Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:50.481Z
Error occurred during asynchronous operation processing: Timestamp must be non-decreasing for series attribute: monitoring/stdout. Invalid point: 2021-11-05T10:06:50.840Z

@kamil-kaczmarek
Copy link
Contributor

kamil-kaczmarek commented Nov 5, 2021

Hey @i008,

Did the same code worked well before upgrade to 1.5 ?

To confirm: is it training on many machines or single-machine with multiple processes?

@i008
Copy link

i008 commented Nov 5, 2021

One local gpu. The (almost) same code worked. I had to adjust the init of the logger.

@kamil-kaczmarek
Copy link
Contributor

@i008,

  • Do you use CUSTOM_RUN_ID or pass the run object around in your code base?
  • Can you share what you modified in the init?

Problem is that async processes write to the same place (stdout in your case) in Neptune and this causes these errors msgs. In practice it does not break your run, these points are simply skipped.

There are few strategies to mitigate that. Happy to chat online if it's ok for you.

@i008
Copy link

i008 commented Nov 5, 2021

Hey @kamil-kaczmarek
Whats the best way to get in touch online? Maybe you can send me a hangout invite @ [email protected] Im avaialble next ~3 hours if that works for you.

@i008
Copy link

i008 commented Nov 5, 2021

I did some further debugging and it seems that this i related to the CLI interface. The problem only occurs while using neptune like that:

    neptune_logger = NeptuneLogger(
        api_key=NEPTUNE_KEY,
        project='projec',
        source_files=["**/*.py", "**/*.ipynb"],

    )
    
    train_defaults = {
        'logger': neptune_logger,
        'gpus': 1,
        'precision': 16

    }
    cli = CLI(
        LitAutoEncoder,
        Data,
        trainer_defaults=train_defaults,
    )

If i do it the regular way ( trainer = , trainer.fit()) things work as expected.

@kamil-kaczmarek
Copy link
Contributor

kamil-kaczmarek commented Nov 5, 2021

Great, thanks for the info. Let's discuss more online

@carmocca
Copy link
Contributor

@shnela @kamil-kaczmarek The deprecated code can now be removed (_signal_deprecated_api_usage)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party Related to a 3rd-party design Includes a design discussion feature Is an improvement or enhancement logger Related to the Loggers ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.