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.

Sign up for GitHub

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

Jump to bottom

Supressing NeptuneUnsupportedType warning if expected metadata not found #21

Merged
merged 7 commits into from
Jan 17, 2024
Merged

Supressing NeptuneUnsupportedType warning if expected metadata not found #21

merged 7 commits into from
Jan 17, 2024

Conversation

SiddhantSadangi
Copy link
Member

No description provided.

@SiddhantSadangi SiddhantSadangi self-assigned this Dec 28, 2023
@SiddhantSadangi SiddhantSadangi added the bug Something isn't working label Dec 28, 2023
AleksanderWWW
AleksanderWWW reviewed Jan 4, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@szysad szysad self-requested a review January 11, 2024 08:36
szysad
szysad reviewed Jan 11, 2024
View reviewed changes
Copy link

@szysad szysad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we add a test that validates that this issue is solved? I can not find any test that validates this. Apart from that LGTM!

src/neptune_sklearn/impl/__init__.py Outdated Show resolved Hide resolved
src/neptune_sklearn/impl/__init__.py Outdated Show resolved Hide resolved
@SiddhantSadangi SiddhantSadangi changed the title Handling NeptuneUnsupportedType error if expected metadata not found Supressing NeptuneUnsupportedType warning if expected metadata not found Jan 15, 2024
@SiddhantSadangi
Copy link
Member Author

@szysad - Added a test

AleksanderWWW
AleksanderWWW reviewed Jan 15, 2024
View reviewed changes
tests/test_e2e.py Outdated Show resolved Hide resolved
tests/test_e2e.py Outdated Show resolved Hide resolved
@AleksanderWWW
Copy link
Contributor

@SiddhantSadangi just thinking - shouldn't we issue a warning that a certain chart couldn't be logged, because it was None? So that the users are not surprised when they don't find the metadata they expected in the run. What do you think?

szysad
szysad reviewed Jan 16, 2024
View reviewed changes
tests/test_e2e.py Outdated Show resolved Hide resolved
@SiddhantSadangi
Copy link
Member Author

@SiddhantSadangi just thinking - shouldn't we issue a warning that a certain chart couldn't be logged, because it was None? So that the users are not surprised when they don't find the metadata they expected in the run. What do you think?

@AleksanderWWW - that's already handled in the code:

neptune-sklearn/src/neptune_sklearn/impl/__init__.py

Line 572 in 1e91d11

print("Did not log learning curve chart. Error: {}".format(e))

@SiddhantSadangi SiddhantSadangi merged commit 0a96ba1 into master Jan 17, 2024
5 checks passed
@SiddhantSadangi SiddhantSadangi deleted the ss/handle_NeptuneUnsupportedType_error branch January 17, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szysad szysad szysad left review comments

@AleksanderWWW AleksanderWWW AleksanderWWW approved these changes

@Raalsky Raalsky Awaiting requested review from Raalsky

Assignees

@SiddhantSadangi SiddhantSadangi

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SiddhantSadangi @AleksanderWWW @szysad