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

add NeptuneLogger [WIP] #1196

Merged
merged 49 commits into from Apr 30, 2021
Merged

add NeptuneLogger [WIP] #1196

merged 49 commits into from Apr 30, 2021

Conversation

kamil-kaczmarek
Copy link
Contributor

@kamil-kaczmarek kamil-kaczmarek commented Apr 24, 2021

Before submitting (checklist)

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contribution guide?
  • Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
  • Did you make sure to update the docs? We use Google format for all the methods and classes.
  • Did you check the docs with make check-docs?
  • Did you write any new necessary tests?
  • Did you check that your code passes the unit tests pytest . ?
  • Did you add your new functionality to the docs?
  • Did you update the CHANGELOG?
  • Did you run colab minimal CI/CD with latest and minimal requirements?

Description

  • added NeptuneLogger
  • added log_artifact to the logger, so that user can log arbitrary files (not limited to images anymore)

Related Issue

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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.

FAQ

Please review the FAQ before submitting an issue:

@pep8speaks
Copy link

pep8speaks commented Apr 24, 2021

Hello @kamil-kaczmarek! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2021-04-30 14:16:15 UTC

@kamil-kaczmarek
Copy link
Contributor Author

Opened draft pull request. Note that we are still polishing it :)

Copy link
Member

@Scitator Scitator left a comment

Choose a reason for hiding this comment

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

the overall implementation looks great!
could you please resolve the codestyle issues?

pip install -r requirements/requireemnts-dev.txt
catalyst-make-codestyle
catalyst-check-codestyle

requirements/requirements-neptune.txt Outdated Show resolved Hide resolved
@Scitator
Copy link
Member

  1. 👍
  2. not sure about the error – the overall implementation looks correct. nevertheless, I am not very familiar with anonymized token and how it should work correctly :)

Co-authored-by: Sergey Kolesnikov <scitator@gmail.com>
@bagxi bagxi self-requested a review April 28, 2021 06:50
@kamil-kaczmarek
Copy link
Contributor Author

quick note: I'll mention the problem with finetune2 test to our devs.

@Scitator Scitator requested a review from ditwoo April 28, 2021 16:24
@kamil-kaczmarek
Copy link
Contributor Author

hey @Scitator, @HubertJaworski from our team will finalize the implemenation today!

@HubertJaworski
Copy link
Contributor

Hi @Scitator

This PR is ready from our end now.

I'm unable to remove [WIP] tag - can you help?

I managed to fix the test - the loggers were not being added.

I took liberty to add neptune's requirements to pip installations in your CICD and add a check for Neptune's integration in check_settings.sh.

I also added some bash functions to improve copy-paste code of pip cleanup & .catalyst creation

@Herudaio
Copy link
Contributor

@Scitator we were missing changes to CHANGELOG and loggers docs, I've updated that. There are two tests failing, but it seems that it's due to some timeout.

@kamil-kaczmarek created the PR and he is off today (🏝🌴🏝) so I cannot update the checklist :)

@Scitator Scitator marked this pull request as ready for review April 30, 2021 14:10
setup.py Outdated Show resolved Hide resolved
Co-authored-by: Sergey Kolesnikov <scitator@gmail.com>
@Scitator Scitator merged commit f07a040 into catalyst-team:master Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants