update for neptune 1.0 #934

kshitij12345 · 2023-02-24T08:36:33Z

No description provided.

skorch/tests/callbacks/test_logging.py

kshitij12345

Lines 260 to 265 in 909f4d5

    
           try: 
        
               # neptune-client=0.9.0+ package structure 
        
               from neptune.new.types import File 
        
           except ImportError: 
        
               # neptune-client>=1.0.0 package structure 
        
               from neptune.types import File

Update based on the decided approach.

BenjaminBossan · 2023-02-24T12:18:49Z

@kshitij12345 Thanks for the PR. Are you with Neptune?

kshitij12345 · 2023-02-24T12:37:21Z

Hi @BenjaminBossan,

Yes. I am with Neptune. Currently the PR is in draft and have asked other members to do a preliminary review.

Once it is ready, I will mark this ready and ping you. Does that work?

Thanks!

BenjaminBossan · 2023-02-24T12:47:04Z

Okay, great, that works. I only asked to ensure that folks at Neptune are okay with dropping support for the old client.

twolodzko · 2023-03-03T09:34:36Z

skorch/callbacks/logging.py

+            try:  # >1.0 package structure
+                from neptune.handler import Handler
+            except ImportError:  # <1.0 package structure
+                from neptune.new.handler import Handler


This can be just

from neptune.handler import Handler

given that the dependency is v >=1.0.0

This is actual logging code which should be compatible with both pre and post 1.0.

Other update where we assume 1.0 structure is in test (where we have updated the requirements to install latest version).

But the requirements say neptune hence >=1.0.0, so there is no backward compatibility assumed.

That is the requirements for development. Installing skorch as a user doesn't install neptune. So we could have users who may use older neptune-client.

Is all the code compatible with older versions? E.g. the use of append instead of log. If not, there is no point in having this check.

I think it's fine to tell users to install the latest version of neptune (except if you know that users are reluctant to upgrade versions). E.g. when this class is initialized, there could be a version check and a helpful error message when the version is too low. WDYT?

skorch/callbacks/logging.py

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

BenjaminBossan · 2023-03-16T10:38:49Z

I got an error in a CI run that seems to be related to Neptune:

>       assert isinstance(
            neptune_run_object.get_structure()['training']['model']['checkpoint'],
            FileSet,
        )
E       AssertionError: assert False
E        +  where False = isinstance(<neptune.attributes.file_set.FileSet object at 0x7f942dcdd850>, <class 'neptune.new.attributes.file_set.FileSet'>)

https://github.com/skorch-dev/skorch/actions/runs/4436004355/jobs/7783884283#step:6:107

My first guess would be that it's caused by a new neptune version. The installed version is neptune-client-1.0.2. It seems this PR might fix the issue, is that right?

kshitij12345 · 2023-03-16T10:45:40Z

Yes, this is due to new version and this PR should fix the issue.

BenjaminBossan · 2023-03-16T10:52:18Z

Great, thanks for letting me know. I don't want to rush this PR, but since CI is currently broken, do you think it will be done soon? Else, I would restrict the neptune version to make CI pass again.

kshitij12345 · 2023-03-16T11:03:06Z

I think the PR is ready. @AleksanderWWW for a final review.

Thank you!

AleksanderWWW

LGTM @kshitij12345 :) The updated imports should resolve the CI issue

kshitij12345 · 2023-03-16T11:09:13Z

@BenjaminBossan this should be ready for your review and merging :)

BenjaminBossan

Thanks a lot for the update. Very nice results on Neptune.

Overall, this is almost good to go, just a few comments, please take a look.

BenjaminBossan · 2023-03-16T14:10:11Z

skorch/callbacks/logging.py

-    >>> import neptune.new as neptune
-    >>> from neptune.new.types import File
+    >>> import neptune
+    >>> from neptune.types import File


I checked the example and found an issue which was already there before but should still be fixed:

- neptune_logger.run["checkpoints].upload_files("./checkpoints") + neptune_logger.run["checkpoints"].upload_files("./checkpoints")

Also, a few lines that start with ... should be >>>.

BenjaminBossan · 2023-03-16T14:21:00Z

skorch/callbacks/logging.py

+            try:  # >1.0 package structure
+                from neptune.handler import Handler
+            except ImportError:  # <1.0 package structure
+                from neptune.new.handler import Handler


Is all the code compatible with older versions? E.g. the use of append instead of log. If not, there is no point in having this check.

I think it's fine to tell users to install the latest version of neptune (except if you know that users are reluctant to upgrade versions). E.g. when this class is initialized, there could be a version check and a helpful error message when the version is too low. WDYT?

BenjaminBossan · 2023-03-16T14:21:59Z

skorch/callbacks/logging.py

+                from neptune.new.handler import Handler
+
+            root_obj = self.run
+            if isinstance(self.run, Handler):


Could you please add a comment here? What is the Handler and why do we sometimes get a Handler and sometimes a Run?

BenjaminBossan

Thanks a lot for keeping the Neptune logger up to date and improving the docs.

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

kshitij12345 added 2 commits February 24, 2023 08:35

update for neptune 1.0

309fb83

update tests for 1.0

2e4952c

kshitij12345 commented Feb 24, 2023

View reviewed changes

skorch/tests/callbacks/test_logging.py Show resolved Hide resolved

update test

909f4d5

kshitij12345 commented Feb 24, 2023

View reviewed changes

kshitij12345 added 3 commits February 27, 2023 11:08

support handler

f3778b9

update for handler

07c9a63

update doc

8e09b51

twolodzko reviewed Mar 3, 2023

View reviewed changes

normandy7 reviewed Mar 15, 2023

View reviewed changes

skorch/callbacks/logging.py Outdated Show resolved Hide resolved

normandy7 reviewed Mar 15, 2023

View reviewed changes

skorch/callbacks/logging.py Show resolved Hide resolved

kshitij12345 and others added 2 commits March 16, 2023 13:43

Update skorch/callbacks/logging.py

fc9ccb7

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

address review

bb1cc0c

kshitij12345 requested a review from normandy7 March 16, 2023 08:14

normandy7 approved these changes Mar 16, 2023

View reviewed changes

BenjaminBossan mentioned this pull request Mar 16, 2023

Add PyTorch v2.0.0 to CI, update docs #938

Merged

kshitij12345 marked this pull request as ready for review March 16, 2023 10:43

AleksanderWWW approved these changes Mar 16, 2023

View reviewed changes

BenjaminBossan requested changes Mar 16, 2023

View reviewed changes

address review

7d8c5ea

BenjaminBossan approved these changes Mar 17, 2023

View reviewed changes

BenjaminBossan merged commit e500dd4 into skorch-dev:master Mar 17, 2023

BenjaminBossan pushed a commit that referenced this pull request Mar 17, 2023

Update NeptuneLogger for neptune 1.0 (#934)

399588e

Co-authored-by: Sabine <sabine.nyholm@neptune.ai>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update for neptune 1.0 #934

update for neptune 1.0 #934

kshitij12345 commented Feb 24, 2023

kshitij12345 left a comment

BenjaminBossan commented Feb 24, 2023

kshitij12345 commented Feb 24, 2023 •

edited

Loading

BenjaminBossan commented Feb 24, 2023

twolodzko Mar 3, 2023

kshitij12345 Mar 3, 2023

twolodzko Mar 3, 2023 •

edited

Loading

kshitij12345 Mar 3, 2023

BenjaminBossan Mar 16, 2023

BenjaminBossan commented Mar 16, 2023

kshitij12345 commented Mar 16, 2023

BenjaminBossan commented Mar 16, 2023

kshitij12345 commented Mar 16, 2023

AleksanderWWW left a comment

kshitij12345 commented Mar 16, 2023

BenjaminBossan left a comment

BenjaminBossan Mar 16, 2023

BenjaminBossan Mar 16, 2023

BenjaminBossan Mar 16, 2023

BenjaminBossan left a comment

	try:
	# neptune-client=0.9.0+ package structure
	from neptune.new.types import File
	except ImportError:
	# neptune-client>=1.0.0 package structure
	from neptune.types import File

update for neptune 1.0 #934

update for neptune 1.0 #934

Conversation

kshitij12345 commented Feb 24, 2023

kshitij12345 left a comment

Choose a reason for hiding this comment

BenjaminBossan commented Feb 24, 2023

kshitij12345 commented Feb 24, 2023 • edited Loading

BenjaminBossan commented Feb 24, 2023

twolodzko Mar 3, 2023

Choose a reason for hiding this comment

kshitij12345 Mar 3, 2023

Choose a reason for hiding this comment

twolodzko Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

kshitij12345 Mar 3, 2023

Choose a reason for hiding this comment

BenjaminBossan Mar 16, 2023

Choose a reason for hiding this comment

BenjaminBossan commented Mar 16, 2023

kshitij12345 commented Mar 16, 2023

BenjaminBossan commented Mar 16, 2023

kshitij12345 commented Mar 16, 2023

AleksanderWWW left a comment

Choose a reason for hiding this comment

kshitij12345 commented Mar 16, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

BenjaminBossan Mar 16, 2023

Choose a reason for hiding this comment

BenjaminBossan Mar 16, 2023

Choose a reason for hiding this comment

BenjaminBossan Mar 16, 2023

Choose a reason for hiding this comment

BenjaminBossan left a comment

Choose a reason for hiding this comment

kshitij12345 commented Feb 24, 2023 •

edited

Loading

twolodzko Mar 3, 2023 •

edited

Loading