Attribute copy

[pkasprzyk]

No description provided.

[shnela]

I'd like to see such test and more meaningful error than what I got now.

        exp["some/path/num"] = 42
        exp["copied/path"] = 11
        exp["copied/path"] = exp["some/path/num"]

self = <[AttributeError("'Integer' object has no attribute 'value'") raised in repr()] Integer object at 0x7fac10319490>
value = <Integer field at "some/path/num">

    def __init__(self, value):
>       self.value = int(value)
E       TypeError: int() argument must be a string, a bytes-like object or a number, not 'Handler'

../../../neptune/new/types/atoms/integer.py:33: TypeError

[pkasprzyk]

yeah, that was really broken... fixed now, I hope -- thanks!

[shnela]

Have I broken this because String does not support copying yet?

        exp["some/path/num"] = "fo9o"
        exp["copied/path"] = 42
        exp["copied/path"] = exp["some/path/num"]

self = <neptune.new.internal.backends.neptune_backend_mock.NeptuneBackendMock.NewValueOpVisitor object at 0x7f0ef5fe2670>
op = CopyAttribute(path=['copied', 'path'], container_id='bdcc8dc7-a2b7-42a9-ba25-209a43888ed7', source_path=['some', 'path', 'num'], source_attr_cls=<class 'neptune.new.attributes.atoms.string.String'>)

    def visit_copy_attribute(self, op: CopyAttribute) -> Optional[Value]:
>       getter = op.source_attr_cls.getter
E       AttributeError: type object 'String' has no attribute 'getter'

Additionally it'd be good to have additional case with copying on existing value (the case recently broken).

[pkasprzyk]

Have I broken this because String does not support copying yet?

yep

Additionally it'd be good to have additional case with copying on existing value

will do

[shnela]

We dropped sort, because we have to make sure that every batch is limited by last copy operation, right?

Why did we sorted in the first place?

[pkasprzyk]

1. yes, exactly
2. that's a great question. @aniezurawski maybe you know?

[aniezurawski]

I do not remember. It is sorted by path so it should not metter. Maybe it was easier to test it this way?

Anyway, are you guys sure it is good idea to change anything in operation_preprocessor? Shouldn't be part-batches preprocessed independently?

[aniezurawski]

I think you could just split a batch by copy operations and reuse existing code for every part-batch. Much simpler and elegant.

[aniezurawski]

Moreover we probably should ack after every part-batch, so maybe single call to execute_operations should just return number operations actually performed and do a single part-batch at the time?

[pkasprzyk]

That would change the idea that in processing a queue batch we do upload (and artifact) ops first -- we would do upload "subbatch", then other "subbatch", and upload again (there's a special comment saying "Upload operations should be done first since they are idempotent").

I'm not if we actually need that -- other than that, I'm perfectly OK with what you suggest.

[aniezurawski]

That would change the idea that in processing a queue batch we do upload (and artifact) ops first

No, it would not. It would still apply but to potentially smaller batches. Just split first, and do upload-related logic later for every part-batch. ;)

[pkasprzyk]

Moreover we probably should ack after every part-batch, so maybe single call to execute_operations should just return number operations actually performed and do a single part-batch at the time?

Right, that sounds good (although the "much simpler" may be lost on me ;))

[pkasprzyk]

done