Proper input value type verification for append method

[Raalsky]

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]

Codecov Report

Base: 71.22% // Head: 71.32% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (6015008) compared to base (36e15a8).
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
+ Coverage   71.22%   71.32%   +0.09%     
==========================================
  Files         281      281              
  Lines       13573    13571       -2     
==========================================
+ Hits         9667     9679      +12     
+ Misses       3906     3892      -14     

Flag	Coverage Δ
e2e	?
unit-pull-request	71.32% <75.00%> (?)

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

Impacted Files	Coverage Δ
src/neptune/handler.py	90.04% <75.00%> (+5.49%)	⬆️
...tune/common/websockets/websocket_client_adapter.py	34.78% <0.00%> (-50.01%)	⬇️
...neptune/legacy/internal/utils/alpha_integration.py	41.66% <0.00%> (-45.84%)	⬇️
...eptune/common/websockets/reconnecting_websocket.py	27.77% <0.00%> (-44.45%)	⬇️
...ternal/threads/hardware_metric_reporting_thread.py	39.13% <0.00%> (-43.48%)	⬇️
...ernal/websockets/reconnecting_websocket_factory.py	57.14% <0.00%> (-42.86%)	⬇️
...tune/legacy/internal/streams/stdstream_uploader.py	46.87% <0.00%> (-40.63%)	⬇️
src/neptune/common/patches/bravado.py	38.88% <0.00%> (-38.89%)	⬇️
src/neptune/legacy/internal/threads/ping_thread.py	42.10% <0.00%> (-36.85%)	⬇️
...eptune/internal/backends/hosted_neptune_backend.py	42.43% <0.00%> (-33.72%)	⬇️
... and 154 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.

[shnela]

Can we change validate_and_transform_to_extend_format to transform_to_extend_format?

[Raalsky]

Done

[shnela]

It sould not be called test_append_many_values_cause_error, now error is caused by non-supported value.
We could test some set, tuple and instance of custom class here as well.

[Raalsky]

Done

[Raalsky]

@normandy7, we had a bug that whenever the user was calling the .append(...) method with list/tuple/list we were raising an exception about provided collection (probably to differentiate it from extend). In the spec, the append method should accept a dictionary (of namespaces) or any other value as a single. There was no need for such a check. Probably no action is needed on your side.

[normandy7]

@normandy7, we had a bug that whenever the user was calling the .append(...) method with list/tuple/list we were raising an exception about provided collection (probably to differentiate it from extend). In the spec, the append method should accept a dictionary (of namespaces) or any other value as a single. There was no need for such a check. Probably no action is needed on your side.

Got it. So this was not possible before:

run["train"].append({"acc": acc, "loss": loss})

but should be OK after this fix?

[Raalsky]

Not exactly.:
run["train"].append({"acc": acc, "loss": loss}):

• Before: Works as expected
• After this PR: Still works

run['a'].append((1, 2)) / run['a'].append([3, 4]) / run['a'].append({5, 6}):

• Before: raises an exception about being collection and invalid type provided
• After this PR: If we'll publish it somehow before 1.0.0 then it would be just translated to string without any warning. After 1.0.0 this will warn with the default NeptuneUnsupportedType warning.

[normandy7]

• After this PR: If we'll publish it somehow before 1.0.0 then it would be just translated to string without any warning. After 1.0.0 this will warn with the default NeptuneUnsupportedType warning.

Ah, makes sense.