-
Notifications
You must be signed in to change notification settings - Fork 63
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
Docstring for pop() #1776
Docstring for pop() #1776
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1776 +/- ##
==========================================
- Coverage 79.52% 73.13% -6.39%
==========================================
Files 371 259 -112
Lines 20270 13658 -6612
==========================================
- Hits 16120 9989 -6131
+ Misses 4150 3669 -481
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -37,6 +37,7 @@ | |||
- Move some `OperationProcessor` implementations to `neptune.core.operation_processors` ([#1760](https://github.com/neptune-ai/neptune-client/pull/1760)) | |||
- Changed handling of too long custom run id ([#1761](https://github.com/neptune-ai/neptune-client/pull/1761)) | |||
- Skip tests that require `GitPython` when not installed ([#1752](https://github.com/neptune-ai/neptune-client/pull/1752)) | |||
- Added docstring for the `pop()` function ([#1776](https://github.com/neptune-ai/neptune-client/pull/1776)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind to cherry pick this changes to dev/1.x
as well?
|
||
Args: | ||
path: Path of the namespace to be removed. | ||
wait: By default, logged metadata is sent to the server in the background. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait
seems not to express the effect properly in the context of pop
. We will wait to all metadata to be removed 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it technically the case that with wait=True
, any queued operations (which may include earlier logging calls) are completed before continuing? And not just this particular pop()
operation.
Conversely, if wait
is used in some call after pop()
, do we not have the same phrasing problem?
run["huge_field"].pop()
run["a"].assign("aaa", wait=True)
I.e. in this case we'll technically wait for deletion rather than sending of metadata. Just wondering if we should adjust this wording globally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a technical point of view, you're absolutely right 😉 . I've started to feel that it looks like being copy-pasted and doesn't match the actual use-case but I'm good as long as it's ok for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming 😃 We'll think of some more accurate and helpful phrasing in a separate update.
Before submitting checklist