Fixed NeptuneLogger support across different versions of neptune package #19131

Raalsky · 2023-12-08T22:13:12Z

What does this PR do?

Before submitting

Was this discussed/agreed 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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist

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

📚 Documentation preview 📚 : https://pytorch-lightning--19131.org.readthedocs.build/en/19131/

codecov · 2023-12-09T07:46:13Z

Codecov Report

Merging #19131 (a2d921e) into master (df869c9) will decrease coverage by 34%.
Report is 2 commits behind head on master.
The diff coverage is 81%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #19131      +/-   ##
==========================================
- Coverage      83%      49%     -34%     
==========================================
  Files         443      435       -8     
  Lines       36764    36616     -148     
==========================================
- Hits        30617    17981   -12636     
- Misses       6147    18635   +12488

src/lightning/pytorch/loggers/neptune.py

carmocca · 2023-12-14T22:36:37Z

src/lightning/pytorch/loggers/neptune.py

+_NEPTUNE_AVAILABLE = bool(RequirementCache("neptune>=1.0")) or bool(RequirementCache("neptune-client>=1.0"))
+_LEGACY_NEPTUNE_CLIENT_AVAILABLE = bool(RequirementCache("neptune-client<1.0"))


These shouldn't be converted to bools. This class overrides str providing a helpful error message to the users. See for instance the use in L227

Suggested change

_NEPTUNE_AVAILABLE = bool(RequirementCache("neptune>=1.0")) or bool(RequirementCache("neptune-client>=1.0"))

_LEGACY_NEPTUNE_CLIENT_AVAILABLE = bool(RequirementCache("neptune-client<1.0"))

_NEPTUNE_AVAILABLE = RequirementCache("neptune>=1.0") or RequirementCache("neptune-client>=1.0")

_LEGACY_NEPTUNE_CLIENT_AVAILABLE = RequirementCache("neptune-client<1.0")

@Raalsky What's the issue with having two separate variables each pointing to each class? (as in the current version)

These shouldn't be converted to bools. This class overrides str providing a helpful error message to the users. See for instance the use in L227

I've been trying during the development and I remember that this leads to two separate messages about the lack of one of the packages. The expected behavior is to raise/warn whenever none of neptune-client/ neptune was installed. This should be covered in: https://github.com/Lightning-AI/pytorch-lightning/pull/19131/files#diff-903c2ecc76f95d71b3f577baf15c47f0e629f6e2a4b201a344177e7908cfccd9R323

What's the issue with having two separate variables each pointing to each class? (as in the current version)

neptune was introduced as a name transition of neptune-client and the long-term target is to get rid of Neptune-client package completely someday. It was introduced as a part of breaking-changes with a release of neptune-client==1.0. neptune-client>=1.0 is just an alias of neptune package and have some breaking-changes in compare to neptune-client<1.0.0. We would like to still support all of the possible environments and package versions. The earlier approach was just "wrong" and had some misunderstandings of how the relationships between each of the packages looked like.

If that's the motivation, then this should work (see the suggestions posted too):

_NEPTUNE_AVAILABLE = RequirementCache("neptune>=1.0") # neptune-client will be gone someday. additionally, version 1.0 had breaking changes to account for _NEPTUNE_CLIENT_AVAILABLE = RequirementCache("neptune-client>=1.0") _LEGACY_NEPTUNE_CLIENT_AVAILABLE = RequirementCache("neptune-client<1.0") _NEPTUNE_GREATER_EQUAL_1_0 = bool(_NEPTUNE_AVAILABLE or _NEPTUNE_CLIENT_AVAILABLE)

@Raalsky thank you for your explanation. As we discontinued support also for older Torch version lest make the cut also here and since next feature release work neptune package only

carmocca · 2023-12-18T12:54:44Z

src/lightning/pytorch/loggers/neptune.py

+_NEPTUNE_AVAILABLE = bool(RequirementCache("neptune>=1.0")) or bool(RequirementCache("neptune-client>=1.0"))
+_LEGACY_NEPTUNE_CLIENT_AVAILABLE = bool(RequirementCache("neptune-client<1.0"))


If that's the motivation, then this should work (see the suggestions posted too):

_NEPTUNE_AVAILABLE = RequirementCache("neptune>=1.0") # neptune-client will be gone someday. additionally, version 1.0 had breaking changes to account for _NEPTUNE_CLIENT_AVAILABLE = RequirementCache("neptune-client>=1.0") _LEGACY_NEPTUNE_CLIENT_AVAILABLE = RequirementCache("neptune-client<1.0") _NEPTUNE_GREATER_EQUAL_1_0 = bool(_NEPTUNE_AVAILABLE or _NEPTUNE_CLIENT_AVAILABLE)

carmocca · 2023-12-18T12:55:24Z

src/lightning/pytorch/loggers/neptune.py

@@ -223,7 +223,7 @@ def __init__(
        prefix: str = "training",
        **neptune_run_kwargs: Any,
    ):
-        if not _NEPTUNE_AVAILABLE and not _NEPTUNE_CLIENT_AVAILABLE:
+        if not _NEPTUNE_AVAILABLE and not _LEGACY_NEPTUNE_CLIENT_AVAILABLE:


Suggested change

if not _NEPTUNE_AVAILABLE and not _LEGACY_NEPTUNE_CLIENT_AVAILABLE:

if not _NEPTUNE_AVAILABLE and not _NEPTUNE_CLIENT_AVAILABLE and not _LEGACY_NEPTUNE_CLIENT_AVAILABLE:

note how the str(_NEPTUNE_AVAILABLE) below is kept, so that it is suggested to install neptune>=1.0

carmocca · 2023-12-18T12:59:01Z

src/lightning/pytorch/loggers/neptune.py

@@ -319,6 +319,9 @@ def _verify_input_arguments(
        if run is not None and not isinstance(run, (Run, Handler)):
            raise ValueError("Run parameter expected to be of type `neptune.Run`, or `neptune.handler.Handler`.")

+        if isinstance(run, Handler) and not _NEPTUNE_AVAILABLE:


Now, in all the places that currently check for _NEPTUNE_AVAILABLE, now you check _NEPTUNE_GREATER_EQUAL_1_0

because if I understood the motivation correctly, you don't care about neptune vs neptune-client but neptune<1.0 vs neptune>=1.0

Borda · 2024-01-10T18:46:35Z

src/lightning/pytorch/loggers/neptune.py

@@ -335,12 +338,12 @@ def __getstate__(self) -> Dict[str, Any]:

    def __setstate__(self, state: Dict[str, Any]) -> None:
        if _NEPTUNE_AVAILABLE:


this seems strange import Neptune even Neptune is not available...

Borda · 2024-01-10T18:47:44Z

src/lightning/pytorch/loggers/neptune.py

@@ -376,12 +379,12 @@ def training_step(self, batch, batch_idx):
    @rank_zero_experiment
    def run(self) -> "Run":
        if _NEPTUNE_AVAILABLE:


Please use a better, more desriptive name, as it is not about Neptune but version...

Borda · 2024-01-10T19:27:24Z

let's update or rebase after #19265 lands 🎏

gitguardian · 2024-01-16T09:24:30Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
-	Generic High Entropy Secret	`78fa3af`	tests/tests_app/utilities/test_login.py	View secret
-	Base64 Basic Authentication	`78fa3af`	tests/tests_app/utilities/test_login.py	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

Raalsky · 2024-02-16T14:49:08Z

Closing in favor of #19265

Fixed neptune support across different versions

d21d9c7

Raalsky requested review from williamFalcon, awaelchli, carmocca and justusschock as code owners December 8, 2023 22:13

github-actions bot added the pl Generic label for PyTorch Lightning package label Dec 8, 2023

Raalsky added 2 commits December 8, 2023 23:14

CHANGELOG

874dc9f

_GET_ROOT_OBJECT_AVAILABLE mocked

f0e22b6

Raalsky requested a review from Borda as a code owner December 9, 2023 07:26

Removed _GET_ROOT_OBJECT indicator

c55b882

awaelchli approved these changes Dec 13, 2023

View reviewed changes

src/lightning/pytorch/loggers/neptune.py Outdated Show resolved Hide resolved

src/lightning/pytorch/loggers/neptune.py Outdated Show resolved Hide resolved

awaelchli added bug Something isn't working logger: neptune community This PR is from the community labels Dec 13, 2023

awaelchli added this to the 2.1.x milestone Dec 13, 2023

Code review

3727b46

AleksanderWWW approved these changes Dec 13, 2023

View reviewed changes

mergify bot added the has conflicts label Dec 14, 2023

Merge branch 'master' into bugfix/neptune-logger-versions-support

a2d921e

mergify bot added has conflicts and removed has conflicts labels Dec 14, 2023

carmocca reviewed Dec 14, 2023

View reviewed changes

Borda approved these changes Dec 14, 2023

View reviewed changes

Borda requested a review from carmocca December 14, 2023 23:06

Raalsky mentioned this pull request Dec 15, 2023

Proper support for Remote Stop and Remote Abort with NeptuneLogger #19130

Merged

7 tasks

carmocca reviewed Dec 18, 2023

View reviewed changes

Borda reviewed Jan 10, 2024

View reviewed changes

Borda requested changes Jan 10, 2024

View reviewed changes

Borda mentioned this pull request Jan 10, 2024

bump: transmission to use neptune only, drop neptune-client #19265

Merged

7 tasks

awaelchli modified the milestones: 2.1.x, 2.2.x Feb 8, 2024

Raalsky closed this Feb 16, 2024

Raalsky deleted the bugfix/neptune-logger-versions-support branch February 16, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed NeptuneLogger support across different versions of neptune package #19131

Fixed NeptuneLogger support across different versions of neptune package #19131

Raalsky commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 9, 2023 •

edited

Loading

carmocca Dec 14, 2023

Borda Dec 14, 2023

carmocca Dec 14, 2023

Raalsky Dec 15, 2023

carmocca Dec 18, 2023 •

edited

Loading

Borda Jan 10, 2024

carmocca Dec 18, 2023 •

edited

Loading

carmocca Dec 18, 2023

carmocca Dec 18, 2023

Borda Jan 10, 2024

Borda Jan 10, 2024

Borda commented Jan 10, 2024

gitguardian bot commented Jan 16, 2024 •

edited

Loading

Raalsky commented Feb 16, 2024

		_NEPTUNE_AVAILABLE = bool(RequirementCache("neptune>=1.0")) or bool(RequirementCache("neptune-client>=1.0"))
		_LEGACY_NEPTUNE_CLIENT_AVAILABLE = bool(RequirementCache("neptune-client<1.0"))

	if not _NEPTUNE_AVAILABLE and not _LEGACY_NEPTUNE_CLIENT_AVAILABLE:
	if not _NEPTUNE_AVAILABLE and not _NEPTUNE_CLIENT_AVAILABLE and not _LEGACY_NEPTUNE_CLIENT_AVAILABLE:

		@@ -335,12 +338,12 @@ def __getstate__(self) -> Dict[str, Any]:

		def __setstate__(self, state: Dict[str, Any]) -> None:
		if _NEPTUNE_AVAILABLE:

Fixed NeptuneLogger support across different versions of neptune package #19131

Fixed NeptuneLogger support across different versions of neptune package #19131

Conversation

Raalsky commented Dec 8, 2023 • edited Loading

What does this PR do?

PR review

codecov bot commented Dec 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmocca Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmocca Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Borda commented Jan 10, 2024

gitguardian bot commented Jan 16, 2024 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Raalsky commented Feb 16, 2024

Raalsky commented Dec 8, 2023 •

edited

Loading

codecov bot commented Dec 9, 2023 •

edited

Loading

carmocca Dec 18, 2023 •

edited

Loading

carmocca Dec 18, 2023 •

edited

Loading

gitguardian bot commented Jan 16, 2024 •

edited

Loading