Add append and extend #1050

PK-Neptune · 2022-10-19T21:05:32Z

No description provided.

github-actions · 2022-10-19T21:08:58Z

Test Results

    1 files -     11     1 suites - 11 35s ⏱️ - 12m 56s
522 tests +    19 519 ✔️ +    16 0 💤 ±  0 3 ❌ +3
522 runs - 5 514 519 ✔️ - 5 423 0 💤 - 94 3 ❌ +3

For more details on these failures, see this check.

Results for commit d6f6563. ± Comparison against base commit 17e9c61.

♻️ This comment has been updated with latest results.

PK-Neptune · 2022-10-21T08:03:18Z

https://github.com/neptune-ai/neptune-client/actions/runs/3290272974/jobs/5433949040

src/neptune/new/internal/utils/__init__.py

tests/neptune/new/test_handler.py

src/neptune/new/handler.py

tests/neptune/new/test_handler.py

shnela · 2022-10-21T13:47:48Z

tests/neptune/new/test_handler.py

+        with self.assertRaises(ValueError):
+            exp["x"].append([])
+        with self.assertRaises(ValueError):
+            exp["x"].append([5, "str"])


Once you fix this code by replacing .append with .extend you'll still have some problems.
Here in this line, the error will be raised when you'll try to log str to FloatSeries, but then you'll have defined FloatSeries under path x which is used later int he code.

I think now it is ok

src/neptune/new/handler.py

shnela · 2022-10-21T14:00:03Z

src/neptune/new/handler.py

                self._container.set_attribute(self._path, attr)
            attr.log(value, step=step, timestamp=timestamp, wait=wait, **kwargs)

+    def _create_new_attribute(self, value):


This method should be moved to MetadataContainer to be consistent with existing define used in assign.

similar to assign - check it

src/neptune/new/handler.py

e2e_tests/standard/test_series.py

shnela · 2022-11-17T16:15:48Z

src/neptune/new/attributes/namespace.py

@@ -73,7 +91,9 @@ def to_dict(self) -> Dict[str, Any]:
        return result

    def assign(self, value: Union[NamespaceVal, dict, Mapping], wait: bool = False):
-        if not isinstance(value, NamespaceVal):
+        if isinstance(value, argparse.Namespace):


This is bugfix for assigning argparse.Namespace type as neptune.Namespace when attribute is already defined.

Due to changes I've made in Handler.assign I had to fix it now.

In fact I had to change Handler.assign implementation again, but bugfix left...

shnela · 2022-11-17T16:26:06Z

src/neptune/new/handler.py

-                attr.process_assignment(value, wait)
-            else:
-                self._container.define(self._path, value, wait)
+            if not attr:


I did two things here:

removed dependency of MetadataContainer.define which now is defined with this very Handler.assign

Implemented flow to be consistent with what we have in: upload, log or _do_extend methods

If attribute does not exist, create attribute based on given value

then assign original (before we used casted value for assignment) value to newly created attribute

then assign original (before we used casted value for assignment) value to newly created attribute

I had to give up this idea, so in case of creating attribute, we're assigning value casted by general cast_value function instead of passing responsibility for casting to attribute.

This will have to be discussed.

shnela · 2022-11-18T20:50:45Z

src/neptune/new/handler.py

+                neptune_value = cast_value(value)
+                attr = ValueToAttributeVisitor(self._container, parse_path(self._path)).visit(neptune_value)
+                self._container.set_attribute(self._path, attr)
+                value = neptune_value  # TODO: we should clean some rules regarding type casting in assignment


Casting value when type is not known (attribute does not exist) and when we already have attribute are completely different things.
We should address this problem soon.

shnela · 2022-11-21T11:34:49Z

src/neptune/new/handler.py

+            ...     ts = df["timestamp"]
+            ...     run["data/example_series"].extend(ys, timestamps=ts)
+        """
+        ExtendUtils.validate_values_for_extend(values, steps, timestamps)


Calling validate_values_for_extend on every level makes extend and append to work in O(n^2) where n is ~~depth of nested structure~~ number of attributes in nested structure.
We can easily fix it, but I don't think it's a problem.

src/neptune/new/attributes/series/series.py

aniezurawski · 2022-11-21T12:43:39Z

src/neptune/new/attributes/series/series.py

-class Series(Attribute, Generic[Val, Data]):
+class Series(Attribute, Generic[ValTV, DataTV, LogOperationTV]):
+    MAX_BATCH_SIZE = None
+    operation_cls: type(LogOperationTV) = None


Isn't it some bad practice? Shouldn't we declare some abstract methods or classmethods instead?
Does access to these variables using self. even work?

Isn't it some bad practice?
I don't know, I think it's common practice in Django, when you override values like template_name, response_class etc.

Does access to these variables using self. even work?
Yes that's what Python does by default:

Tries to to obtain instance variable stored in slots or internal __dict__

falls back to class attributes

Doesn't abstract or class method sound like a better idea for you? You don't even have default values here and nothing forces you to override them in derived class.

I get your point, but what you're suggesting looks like Java in Python and I don't feel it.

Maybe I just used to fucked up Python patterns, because to me even overriding __init_sublcass__ like here looks like a solution in this case...

I don't know. Maybe @Raalsky has some opinion on writing Python.

I may miss something but the class properties are pretty useful things like the ground for descriptors and dataclasses fields.

Doesn't abstract or class method sound like a better idea for you? You don't even have default values
here and nothing forces you to override them in derived class.

You can as well override any abstract method, class method etc. (I think so :D).

I don't wanna dig deeply into this PR but it looks ok as a first impression.

@Raalsky I agree with class properties being pretty useful in general but here they are used as something to be overriden in subclasses but python does not force you to do it and they don't have any reasonable default. If you add new subclass in future would it be obvious for you to override them?

You can as well override any abstract method, class method etc. (I think so :D).

This is my point. We want them to be overriden.

here they are used as something to be overriden in subclasses but python does not force you to do it and they don't have any reasonable default. If you add new subclass in future would it be obvious for you to override them?

@aniezurawski

Are you ok with __init_sublcass__ as solution for your problem?

I never heard of it earlier so I have no intuition about possible use cases and best practices but it looks like a good idea.

Ok done. We'll get immediate runtime error when we try to run code with Series subclass without properly overridden values.

src/neptune/new/attributes/series/series.py

src/neptune/new/attributes/series/string_series.py

src/neptune/new/attributes/namespace.py

src/neptune/new/metadata_containers/metadata_container.py

src/neptune/new/attributes/series/file_series.py

src/neptune/new/handler.py

src/neptune/new/attributes/namespace.py

aniezurawski · 2022-12-05T13:00:05Z

src/neptune/new/handler.py

+        if isinstance(value, Namespace) or is_dict_like(value):
+            return {k: ExtendUtils.validate_and_transform_to_extend_format(v) for k, v in value.items()}
+        elif is_collection(value):
+            raise NeptuneUserApiInputException(


Was custom exception a requirement from spec? Because I think ValueError is standard python exception for such cases and I think it allows you to include some custom message.

I think it was discussed between PK and Mycek and custom exception here looks reasonable, we're changing the API and want to explicitly inform user that he uses new endpoint the wrong way.

Can you confirm @Herudaio

src/neptune/new/attributes/series/file_series.py

src/neptune/new/attributes/series/string_series.py

aniezurawski · 2022-12-06T11:20:14Z

src/neptune/new/handler.py

-                neptune_value = cast_value(value)
-                attr.process_assignment(neptune_value, wait)
+                if isinstance(value, Handler):
+                    value = ValueCopy(value)


Why not cast_value?

No, we just override value when value is of instance Handler, if not we use original one.

aniezurawski · 2022-12-06T11:23:39Z

src/neptune/new/attributes/series/series.py

        step: Optional[float] = None,
        timestamp: Optional[float] = None,
        wait: bool = False,
        **kwargs,
    ) -> None:
+        """log is a deprecated method, this code should be removed in future"""


Do we want some warning?

PK-Neptune force-pushed the pk/NPT-12400 branch 2 times, most recently from 6676a7f to 3fda587 Compare October 20, 2022 12:41

shnela suggested changes Oct 21, 2022

View reviewed changes

shnela reviewed Oct 27, 2022

View reviewed changes

e2e_tests/standard/test_series.py Outdated Show resolved Hide resolved

e2e_tests/standard/test_series.py Outdated Show resolved Hide resolved

PK-Neptune force-pushed the pk/NPT-12400 branch 2 times, most recently from 2cfe508 to f42f839 Compare November 2, 2022 10:22

shnela force-pushed the pk/NPT-12400 branch 2 times, most recently from 2717774 to e28b492 Compare November 14, 2022 13:31

shnela force-pushed the pk/NPT-12400 branch from 81b22c2 to b5c0f5a Compare November 16, 2022 13:48

shnela reviewed Nov 17, 2022

View reviewed changes

shnela force-pushed the pk/NPT-12400 branch from 61d10e3 to 30940e1 Compare November 17, 2022 18:37

shnela reviewed Nov 18, 2022

View reviewed changes

shnela reviewed Nov 21, 2022

View reviewed changes

aniezurawski requested changes Nov 21, 2022

View reviewed changes

shnela force-pushed the pk/NPT-12400 branch 3 times, most recently from e654a2b to d6f6563 Compare December 2, 2022 11:33

aniezurawski requested changes Dec 5, 2022

View reviewed changes

aniezurawski reviewed Dec 5, 2022

View reviewed changes

aniezurawski requested changes Dec 6, 2022

View reviewed changes

src/neptune/new/attributes/series/file_series.py Outdated Show resolved Hide resolved

src/neptune/new/attributes/series/string_series.py Outdated Show resolved Hide resolved

aniezurawski reviewed Dec 6, 2022

View reviewed changes

PK-Neptune and others added 5 commits December 6, 2022 12:38

NPT-12400: append and extend

d78f115

NPT-12400: more tests

74d5fb2

Fix tests

3698fe6

Adapt Series attribute to list of steps and timestamps

e83bfaa

Delegate Container.define call to Handler

e3c7372

shnela and others added 19 commits December 6, 2022 12:38

Implement append using extend

0df9c95

Add docstring

c2f2bb7

Keep maximum one nested level for append

270469f

Small fixes

c796746

Create ExtendUtils

272cdd3

Adjust stack_level

d5eae36

Allow arbitrary deep nested level for series

608cb74

Clean code

74a6b3f

Add ExtendDictT

256e543

Expect key in nested dicts to be str

b6c5873

PR fixes

ec8a6b7

CR fixes

1ba405e

CR fixes

c311b29

Post review fixes

5ea30ed

Protect Series attributes overriden

558c70b

Use __init_subclass__ attributes

6280108

Fix tests

6ec7434

Fix copy-paste

2027c8e

PR fixes

3c23b40

shnela force-pushed the pk/NPT-12400 branch from b5924a7 to 3c23b40 Compare December 6, 2022 11:38

PR fixes

d1db6a8

aniezurawski previously approved these changes Dec 6, 2022

View reviewed changes

Fix tests

e7732fd

shnela dismissed aniezurawski’s stale review via e7732fd December 6, 2022 12:42

aniezurawski approved these changes Dec 6, 2022

View reviewed changes

shnela approved these changes Dec 6, 2022

View reviewed changes

shnela merged commit 17b99cd into master Dec 6, 2022

shnela deleted the pk/NPT-12400 branch December 6, 2022 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add append and extend #1050

Add append and extend #1050

PK-Neptune commented Oct 19, 2022

github-actions bot commented Oct 19, 2022 •

edited

PK-Neptune commented Oct 21, 2022

shnela Oct 21, 2022

PK-Neptune Oct 27, 2022

shnela Oct 21, 2022

PK-Neptune Oct 27, 2022

shnela Nov 17, 2022

shnela Nov 18, 2022

shnela Nov 17, 2022

shnela Nov 21, 2022 •

edited

shnela Nov 18, 2022

shnela Nov 21, 2022 •

edited

aniezurawski Nov 21, 2022

shnela Nov 30, 2022

aniezurawski Nov 30, 2022

shnela Dec 1, 2022

Raalsky Dec 1, 2022 •

edited

aniezurawski Dec 5, 2022 •

edited

shnela Dec 5, 2022

aniezurawski Dec 5, 2022

shnela Dec 5, 2022

aniezurawski Dec 5, 2022

shnela Dec 5, 2022

aniezurawski Dec 6, 2022

shnela Dec 6, 2022

aniezurawski Dec 6, 2022

shnela Dec 6, 2022

Add append and extend #1050

Add append and extend #1050

Conversation

PK-Neptune commented Oct 19, 2022

github-actions bot commented Oct 19, 2022 • edited

Test Results

PK-Neptune commented Oct 21, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnela Nov 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnela Nov 21, 2022 • edited

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

Raalsky Dec 1, 2022 • edited

Choose a reason for hiding this comment

aniezurawski Dec 5, 2022 • edited

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

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

github-actions bot commented Oct 19, 2022 •

edited

shnela Nov 21, 2022 •

edited

shnela Nov 21, 2022 •

edited

Raalsky Dec 1, 2022 •

edited

aniezurawski Dec 5, 2022 •

edited