SIMPLE-6124 annotations support #85

PatrikMosko · 2024-02-20T12:01:29Z

Added basic annotations support.

Most notable additions:
Lab.annotations()
Lab.get_annotation_by_id()
Lab.create_annotation()
Lab.remove_annotations()
Lab.remove_annotation()

Updated functionalities:
Lab.update_lab()
Lab.import_lab()

Other changes:

removed functools.total_ordering from Element objects
improved internal documentation (docstrings)

…sted

…tters

added docstrings to annotation properties fixed issues with syncing

added as_dict method to annotations

PatrikMosko · 2024-02-20T12:02:18Z

Lab.update_lab() and Lab.import_lab() is currently the only untested part of the PR.

tmikuska · 2024-02-20T13:03:16Z

virl2_client/models/lab.py

@@ -617,7 +658,7 @@ def remove_node(self, node: Node | str, wait: bool | None = None) -> None:
        """
        Remove a node from the lab.

-        If you have a node object, you can also simply do::


I don't think this was a typo.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks It was not.

I will revert it then, after you finish review.

tmikuska · 2024-02-20T13:04:05Z

is there a simple branch with some annotation tests?

PatrikMosko · 2024-02-20T13:06:39Z

Sadly there isn't, not sure if you mean unit tests or any tests...
We only have API annotation tests, possibly some UI tests.

daniel-valent

I'll review the annotation parts when you are done redoing them, ping me then pls&ty.

daniel-valent · 2024-02-20T14:09:33Z

virl2_client/models/lab.py

+        """
+        Return the list of nodes in the lab.
+
+        :returns: A list of Node objects.


nodes -> annotations

daniel-valent · 2024-02-20T14:17:52Z

virl2_client/models/lab.py

+        Creates a lab annotation
+
+        :param type: type of annotation (rectangle, ellipse, line or text)
+        :returns: the created annotation


We've unified the formatting of docstrings to use imperative mood ("Create", not "Creates") and proper capitalization and punctuation in the :param:s etc.
Not exactly critical, but I'd rather not revert to the standardless lawlessness just yet.

reverted some minor docstring changes fixed various issues mostly related to Lab methods and syncing

tmikuska · 2024-02-21T11:52:17Z

virl2_client/models/annotation.py

+
+    AnnotationTypeString = Literal["text", "line", "ellipse", "rectangle"]
+    AnnotationType = Union[
+        Annotation,


probably just to shut linters up

Suggested change

Annotation,

"Annotation",

tmikuska · 2024-02-21T11:53:54Z

virl2_client/models/annotation.py

+        return annotation_map[annotation_type] & ANNOTATION_PROPERTY_MAP[_property] > 0
+
+    @locked
+    def as_dict(self) -> dict[str, str]:


I don't think that we need as_dict in PCL. We should deprecate it in interface.py and link.py (not in this PR).

tmikuska · 2024-02-21T11:57:38Z

virl2_client/models/annotation.py

+# --X-: ellipse
+# -X--: line
+# X---: text
+ANNOTATION_PROPERTY_MAP = {


I'd remove these mappings too. We don't need as_dict, get_default_property_values or is_valid_property methods.

tmikuska · 2024-02-21T12:01:29Z

virl2_client/models/lab.py

+
+        # create POST json with default annotation property values
+        # override some values by new, expected ones
+        annotation_data = Annotation.get_default_property_values(annotation_type)


Let's server provide the defaults. If that's not possible, we should really consider changing the API. It makes no sense to require defaults (and probably not just in this API endpoint).

Every property for each ann. type is required therefore when creating new annotation, you have to provide all of its properties.

I created map of default values, which are the same default values than the one you get when creating annotation through ui.

I made this so you dont have to insert all (required) property values when creating new annotation through PCL, so now you can just do: lab.create_annotation("rectangle")

i'd prefer to have a set of functions which defines the properties for annotation, not defined on the backend but locally in the PCL. This is about abstracting complexity and making the use of the API easier by providing "quality of life" functions for the user of the PCL.

Annotation.get_default_properties(annotation_type) as Patrik proposes which define sane defaults

Annotation.set_default_properties(annotation_type, defaults) which provides additional values the user wants for this annotation type, they will be merged with the default properties

Annotation.clear_properties(annotation_type) which will clear the user properties

When the user then creates annotations, the union of the defaults and the user provided properties and the create function are passed to the backend API call to have a complete set of attributes/properties

Example

"defaults for rect": { bg_color="#FFFFFFFF", border_color="#808080FF", border_style="solid", border_thickness="1", corner_radius=0, layer=0 }

user calls Annotation.create_rect(0,0, 100, 100) -> properties taken from defaults
user calls Annotation.create_rect(0,0, 100, 100, border_color="#ff000000") -> red rectangle
user calls
Annotations.set_default_properties(Rectangle, border_color="#00ff0000")
Annotations.create_rect(0,0, 100,100) -> blue rectangle
Annotations.create_rect(100,100, 100,100) -> another blue rectangle
Annotations.clear_default_properties(Rectangle)
Annotations.create_rect(200,200, 100,100) -> rectangle is black again

This way, the actual creation ALWAYS provides a complete set of defaults and it's easy for the user to control the properties of annotations without having to repeat themselves for every object created. And the backend does (and should) not have any say in what is a default... the API wants and gets a complete set of properties for every annotation created.

the reset to defaults is clearly something that we don't do in PCL (we don't even have such options in CML maybe except of sync from a node def)

we do not reset annotations to default in UI either

do we really think that this would be useful? why don't we just delete and create a new annotation instead?

While I like the idea of having something like "default properties overlay", in reality its not really bringing that much of a QoL change.
While now you have to always provide your custom defaults as a second argument to "create" method (Lab.create_annotation("rectangle", user_defined_defaults)), with your proposal in-place, it would be only one (Lab.create_annotation("rectangle")).

I dont think saving one dict var with user-defined defaults and sending one extra parameter in each create call is that much of a pain, sometimes more abstraction hurts.

Another issue is we run async, which means there's a miniscule chance that before you send the request with expected defaultsA, other method might update these class-level defaults to defaultsB and they will be used for the first call, expecting defaultsA, too.

Therefore sending the (user-defined subset of) defaults explicitly, while not being overly painful, gives you more control over what is happening.

…e annotation body minor tweak of AnnotationType typings

daniel-valent · 2024-02-26T09:45:05Z

virl2_client/models/annotation.py

-        self,
-        annotation_data: dict[str, Any],
-        push_to_server: bool = False,
+        self, annotation_data: dict[str, Any], push_to_server: bool = True


I think we should drop the push_to_server parameter in the entirety of the PCL and just make the update functions always push to server, since there is really no situation where the user would want to not push to server. We only ever do that internally, so it should be its own separate private function. @tmikuska can we discuss that/make a ticket for it? (You can leave it here for now I think)

I get the idea of WHY we initially wanted to give the user a possibility to handle the objects only locally, but in reality it makes little to no sense, so as you are saying, I think we should always try to force push it to server, which is something most of the users will expect to happen automatically.

If this is true (and I think it is) then the entire "offline/async" mode should be removed from the PCL as it makes things overly complicated for little to no gain as people are likely not using it in this async mode but rather expect things to be synchronous.

raised a new ticket for this

* SIMPLE-5780 replace change test dir fixture (#54) * SIMPLE-5377 PCL general improvements (#59) * SIMPLE-5953 pcl update for smbios parameters (#61) * SIMPLE-5974 Support for locked_compute_id (#64) * SIMPLE-5806 PCL support for multiple config files (#60) * SIMPLE-5806 fix for a missed bug (#65) * SIMPLE-5949 Deprecated licensing certificate support in PCL (#66) * updated pyproject.toml and poetry.lock; versions: poetry-1.4, lock for… (#68) * SIMPLE-6084 Fixed regressions in create_node method to preserve compatibility with older CML releases * SIMPLE-6020 sped up syncing operational by removing unneeded API calls (#72) * SIMPLE-5747 added .iol files to list of accepted image files (#71) * SIMPLE-5829 Backported fix for missing image extension * SIMPLE-6087 creating a lab no longer raises a warning (#74) * SIMPLE-6105 node.remove no longer always raises NodeNotFound (#75) * SIMPLE-6073: Avoid using unneeded pyats features when parsing our generated testbed file, which uses none of them. Refactor checking whether pyats is installed. (#76) * SIMPLE-6168 Added missing copyright info, fixed invalid copyright year * SIMPLE-6204 rename locked compute to pinned (#79) * SIMPLE-6093 Implemented controller property (#82) * SIMPLE-6285 Improvements to multiple node configuration API (#83) * SIMPLE-6124 Annotations support (#85) * SIMPLE-6428 Fixed annotation update on lab sync (#89)

PatrikMosko added 4 commits February 19, 2024 17:34

SIMPLE-6124: init commit; added unfinished annotations support - unte…

b784cff

…sted

SIMPLE-6124: fixed initial issues with annotation property getters/se…

825720f

…tters

SIMPE-6124: fixed annotation getters/setters

37913aa

added docstrings to annotation properties fixed issues with syncing

SIMPLE-6124: minor internal documentation changes

9663109

added as_dict method to annotations

PatrikMosko requested review from rschmied, tmikuska and daniel-valent February 20, 2024 12:01

daniel-valent changed the title ~~Simple 6124 annotations support~~ SIMPLE-6124 annotations support Feb 20, 2024

tmikuska reviewed Feb 20, 2024

View reviewed changes

daniel-valent reviewed Feb 20, 2024

View reviewed changes

PatrikMosko added 2 commits February 21, 2024 02:33

SIMPLE-6124: refactorred annotation properties

84f3498

reverted some minor docstring changes fixed various issues mostly related to Lab methods and syncing

SIMPLE-6124: running pre-commit

fa34f9d

PatrikMosko requested review from daniel-valent and tmikuska February 21, 2024 09:26

tmikuska requested changes Feb 21, 2024

View reviewed changes

SIMPLE-6124: create_annotation now takes input data from http respons…

ac74d14

…e annotation body minor tweak of AnnotationType typings

PatrikMosko requested a review from tmikuska February 23, 2024 11:58

daniel-valent approved these changes Feb 26, 2024

View reviewed changes

Cleanup

055951f

tmikuska force-pushed the SIMPLE-6124-annotations-support branch from 001c698 to 055951f Compare March 1, 2024 12:35

tmikuska merged commit 0303cd9 into dev Mar 1, 2024

tmikuska deleted the SIMPLE-6124-annotations-support branch March 1, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIMPLE-6124 annotations support #85

SIMPLE-6124 annotations support #85

PatrikMosko commented Feb 20, 2024

PatrikMosko commented Feb 20, 2024

tmikuska Feb 20, 2024

daniel-valent Feb 20, 2024

PatrikMosko Feb 20, 2024

tmikuska commented Feb 20, 2024

PatrikMosko commented Feb 20, 2024

daniel-valent left a comment

daniel-valent Feb 20, 2024

daniel-valent Feb 20, 2024

tmikuska Feb 21, 2024

tmikuska Feb 21, 2024

tmikuska Feb 21, 2024

tmikuska Feb 21, 2024 •

edited by PatrikMosko

Loading

PatrikMosko Feb 21, 2024

rschmied Feb 29, 2024

tmikuska Feb 29, 2024

PatrikMosko Feb 29, 2024

PatrikMosko Feb 29, 2024

daniel-valent Feb 26, 2024

PatrikMosko Feb 26, 2024 •

edited

Loading

rschmied Feb 29, 2024

tmikuska Feb 29, 2024

SIMPLE-6124 annotations support #85

SIMPLE-6124 annotations support #85

Conversation

PatrikMosko commented Feb 20, 2024

Added basic annotations support.

PatrikMosko commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmikuska commented Feb 20, 2024

PatrikMosko commented Feb 20, 2024

daniel-valent left a 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

tmikuska Feb 21, 2024 • edited by PatrikMosko Loading

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

PatrikMosko Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmikuska Feb 21, 2024 •

edited by PatrikMosko

Loading

PatrikMosko Feb 26, 2024 •

edited

Loading