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

Made Run, Model, ModelVersion and Project made first class citizens #1246

Merged
merged 29 commits into from Mar 7, 2023

Conversation

Raalsky
Copy link
Contributor

@Raalsky Raalsky commented Feb 18, 2023

This allows us to call Run and any other object in-place of init_run:

with Run() as run:
    run['a'] = 99.9

Before submitting checklist

  • Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
  • Did you asked the docs owner to review all the updated user-facing interfaces?

@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Patch coverage has no change and project coverage change: -71.52 ⚠️

Comparison is base (c547756) 71.51% compared to head (7f8a3ad) 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #1246       +/-   ##
==========================================
- Coverage   71.51%   0.00%   -71.52%     
==========================================
  Files         282     277        -5     
  Lines       13643   13583       -60     
==========================================
- Hits         9757       0     -9757     
- Misses       3886   13583     +9697     
Flag Coverage Δ
e2e ?
unit ?
unit-pull-request 0.00% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/neptune/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
.../neptune/internal/backends/neptune_backend_mock.py 0.00% <ø> (-38.39%) ⬇️
.../neptune/metadata_containers/metadata_container.py 0.00% <0.00%> (-87.31%) ⬇️
src/neptune/metadata_containers/model.py 0.00% <0.00%> (-94.12%) ⬇️
src/neptune/metadata_containers/model_version.py 0.00% <0.00%> (-91.67%) ⬇️
src/neptune/metadata_containers/project.py 0.00% <0.00%> (-90.48%) ⬇️
src/neptune/metadata_containers/run.py 0.00% <0.00%> (-87.18%) ⬇️
src/neptune/envs.py 0.00% <0.00%> (-100.00%) ⬇️
src/neptune/constants.py 0.00% <0.00%> (-100.00%) ⬇️
src/neptune/common/envs.py 0.00% <0.00%> (-100.00%) ⬇️
... and 237 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Raalsky Raalsky force-pushed the rj/via-constructor branch 2 times, most recently from 404e23e to 09e4bc3 Compare February 20, 2023 09:51
@twolodzko
Copy link
Contributor

I didn't review the implementation, just looked at the API, and see no reason why we shouldn't have it.

Copy link
Contributor

@AleksanderWWW AleksanderWWW left a comment

Choose a reason for hiding this comment

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

Looks good to me on the code front. Doc strings probably need to be updated, as they only point to using init_* aliases

@Raalsky Raalsky requested a review from normandy7 March 7, 2023 11:27
@Raalsky
Copy link
Contributor Author

Raalsky commented Mar 7, 2023

@normandy7, this is mostly our internal code deduplication etc. but as a result, this adds a feature to use Run/Model/ModelVersion/Project in addition to standard init_run. There is no requirement to expose it currently in docs/docstrings. For example:

with Run() as run:
    run['a'] = 99.9

# or

run = Run()

Copy link
Collaborator

@normandy7 normandy7 left a comment

Choose a reason for hiding this comment

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

I agree no docs action should be required for now. We may update the class docstrings in the future, should we decide to "officially" include the constructor approach in the API. Thanks!

@Raalsky Raalsky merged commit e0da79e into master Mar 7, 2023
@Raalsky Raalsky deleted the rj/via-constructor branch March 7, 2023 16:26
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

4 participants