feat: added method for returning full plan object #34

adrianLIrobotics · 2023-05-12T07:12:14Z

I added another method for returning the whole plan for later parsing in the AC/AS.

Many thanks!

Kapim

The gateway_get_full_plan is almost identical to the gateway_get_plan (the only difference is that the latter only returns one ActionPlanID). Can you rewrite the gateway_get_plan method to utilize the gateway_get_full_plan?

Kapim · 2023-05-12T08:13:42Z

era_5g_client/client.py

@@ -254,6 +254,35 @@ def gateway_get_plan(self, taskid: str, resource_lock: bool, robot_id: str) -> s
        except KeyError as e:
            raise FailedToConnect(f"Could not get the plan: {e}")

+
+    def gateway_get_full_plan(self, taskid: str, resource_lock: bool, robot_id: str) -> str:


The method does not return a string but a response (a dict probably?). Moreover, should it return the whole response? I think it would be better if we could create a data class that will describe the plan and return it (similar to MiddlewareInfo in https://github.com/5G-ERA/era-5g-client/blob/main/era_5g_client/dataclasses.py).

adrianLIrobotics · 2023-05-17T11:52:20Z

Hello Michal, Bartosz, can you check the dataclasses file and the parser_middleware_plan_info in client.py

The idea is that the response from the middleware will be translated into a dataclass object defined with the middleware response. Please do not merge as i still want to do proper testing :)

Artonus

@adrianLIrobotics, the logic looks good to me, but please correct the changes mentioned in the comments. On top of this, please make sure to address the changes that @Kapim has requested.
Thanks!

Artonus · 2023-05-17T12:52:59Z

era_5g_client/dataclasses.py

+    # Is currently been used by the robot
+    enabled: bool
+
+    def build_api_endpoint(self, path: str) -> str:


@adrianLIrobotics is this function needed here? I don't see how the topic needs to build an API endpoint :)

Artonus · 2023-05-17T12:54:00Z

era_5g_client/dataclasses.py

+    # Onboarded time for network application.
+    onboardedTime: str
+
+    def build_api_endpoint(self, path: str) -> str:


Same as in the previous class

Artonus · 2023-05-17T12:54:09Z

era_5g_client/dataclasses.py

+    # Services of the action
+    services : list[MiddlewareServiceInfo]
+
+    def build_api_endpoint(self, path: str) -> str:


Artonus · 2023-05-17T12:55:26Z

era_5g_client/dataclasses.py

+    # List of actions in the task
+    actionSequence: list[MiddlewareActionInfo]
+
+    def build_api_endpoint(self, path: str) -> str:


Same here, this function does not need to be duplicated this many times

Artonus · 2023-05-18T09:44:19Z

era_5g_client/client.py

+                topicPubData = service["rosTopicsPub"]
+                topicSubData = service["rosTopicsSub"]
+                for z in range(0,len(topicPubData)):
+                    rosTopicsPub.append(MiddlewareRosTopicModel(topicPubData["name"],topicPubData["type"],topicPubData["description"],topicPubData["enabled"] ))


Can a constructor be created in the dataclass to accept the dictionary? Then in this constructor, we could extract all the necessary data. It would make this section of the code much cleaner and easy to read.

I think we should keep the constructor as it is but either implement some "from_dict" static method which will instantiate the object from the dictionary or use, e.g., the dacite.from_dict method to create the instance rosTopicPub.append(dacite.from_dict(data_class=MiddlewareRosTopicModel, data=topicPubData)). @ZdenekM, what would be the best way to instantiate the dataclass model here?

But I definitely agree with @Artonus that we should make this section more cleaner

@Kapim If you need from_dict method for dataclasses, takes a look here: https://github.com/s-knibbs/dataclasses-jsonschema/ - pretty cool, I used it in arcor2.

And because properties in a dictionary are named in the camelCase convention and properties in dataclasses will be in snake_case, we will need to convert the dictionary between those with this: https://github.com/nficano/humps.

Hmm, I looked better, and it seems there is a mix of camelCase and PascalCase (ServiceType).

Artonus · 2023-05-18T09:48:06Z

era_5g_client/dataclasses.py

+    # The name of the task
+    name: str
+    # Block semantic planning, use predefined action seq.
+    ReplanActionPlannerLocked: bool


@adrianLIrobotics I believe that these names don't follow the correct python name format, and this is why you can't get the CI build passing.
@Kapim can you confirm it?

Yes, you should follow the PEP 8 style guide, i.e., the variable names should be snake_case formatted (lowercase with words separated by underscores). Only classes should use the PascalCase format.

There are more problems :) But most of them can be solved by using autoformatting (./pants fmt ::). After passing this stage of check, there will be type checking (mypy) which will (possibly) reveal another set of problems.

Kapim

I have added a few more comments

Kapim · 2023-05-18T10:29:04Z

era_5g_client/client.py

+    def parser_middleware_plan_info(self, response: dict) -> MiddlewarePlanInfo:
+        actionSequenceData = response['ActionSequence']
+        action_list = []
+        for x in range(0,len(actionSequenceData)):


It is more "Pythonic" to iterate through iterable like this for action in actionSequenceData:

Also, action_list = [] should have a type annotation.

Kapim · 2023-05-18T10:29:35Z

era_5g_client/client.py

+            action = actionSequenceData[x]
+            service_list = []
+            serviceData = action["services"]
+            for y in range(0,len(serviceData)):


same as above, please use for service in serviceData:

Add type annotation to service_list = [].

Kapim · 2023-05-18T10:31:47Z

era_5g_client/client.py

+                rosTopicsSub = []
+                topicPubData = service["rosTopicsPub"]
+                topicSubData = service["rosTopicsSub"]
+                for z in range(0,len(topicPubData)):


Same as above. Moreover, is the topicPubData a list or something? Because inside the cycle, you are not iterating through it and only extracting the data from it.

Kapim · 2023-05-18T10:39:01Z

era_5g_client/client.py

+                topicPubData = service["rosTopicsPub"]
+                topicSubData = service["rosTopicsSub"]
+                for z in range(0,len(topicPubData)):
+                    rosTopicsPub.append(MiddlewareRosTopicModel(topicPubData["name"],topicPubData["type"],topicPubData["description"],topicPubData["enabled"] ))


I think we should keep the constructor as it is but either implement some "from_dict" static method which will instantiate the object from the dictionary or use, e.g., the dacite.from_dict method to create the instance rosTopicPub.append(dacite.from_dict(data_class=MiddlewareRosTopicModel, data=topicPubData)). @ZdenekM, what would be the best way to instantiate the dataclass model here?

Kapim · 2023-05-18T10:39:31Z

era_5g_client/client.py

+                topicPubData = service["rosTopicsPub"]
+                topicSubData = service["rosTopicsSub"]
+                for z in range(0,len(topicPubData)):
+                    rosTopicsPub.append(MiddlewareRosTopicModel(topicPubData["name"],topicPubData["type"],topicPubData["description"],topicPubData["enabled"] ))


But I definitely agree with @Artonus that we should make this section more cleaner

Kapim · 2023-05-18T10:47:56Z

era_5g_client/dataclasses.py

+    # The name of the task
+    name: str
+    # Block semantic planning, use predefined action seq.
+    ReplanActionPlannerLocked: bool


Yes, you should follow the PEP 8 style guide, i.e., the variable names should be snake_case formatted (lowercase with words separated by underscores). Only classes should use the PascalCase format.

ZdenekM · 2023-05-19T06:33:34Z

@adrianLIrobotics Before making a commit, please run ./pants fmt :: (it will sort imports and format the code), then ./pants lint :: (it will run flake8 and black tools and report possible problems), and finally ./pants check :: (runs mypy tool, which performs static analysis of the code and types). Thanks!

ZdenekM · 2023-05-19T06:45:01Z

@adrianLIrobotics Also, please merge commits into one so we have a clear and comprehensible history. Many thanks!

Kapim · 2023-09-20T08:26:31Z

@adrianLIrobotics do you plan to work on this in the near future?

ZdenekM · 2024-04-23T06:28:53Z

Closing for no activity.

feat: added method for returning full plan object

9bb9e5b

adrianLIrobotics added the enhancement New feature or request label May 12, 2023

adrianLIrobotics requested a review from Kapim May 12, 2023 07:12

Kapim requested changes May 12, 2023

View reviewed changes

Kapim reviewed May 12, 2023

View reviewed changes

adrianLIrobotics added 6 commits May 12, 2023 12:27

feat: added data classes for middleware

fdc0d77

feat: updated requirements.txt

080140a

Fix dataclass model

f786e4e

Update client.py

6e66cb8

Update dataclasses.py

066e4d3

Update client.py

3a763ad

adrianLIrobotics requested a review from Artonus May 17, 2023 11:50

Artonus requested changes May 18, 2023

View reviewed changes

Kapim requested changes May 18, 2023

View reviewed changes

adrianLIrobotics added 3 commits May 24, 2023 10:29

removed build_api_endpoints

d576743

Update client.py

46cb9e5

Update client.py

10ab107

Artonus linked an issue Jun 14, 2023 that may be closed by this pull request

Extra function for gateway_get_plan returning dictionary with all data #32

Open

Artonus mentioned this pull request Jun 14, 2023

Extra function for gateway_get_plan returning dictionary with all data #32

Open

ZdenekM closed this Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added method for returning full plan object #34

feat: added method for returning full plan object #34

adrianLIrobotics commented May 12, 2023

Kapim left a comment

Kapim May 12, 2023

adrianLIrobotics commented May 17, 2023

Artonus left a comment

Artonus May 17, 2023

Artonus May 17, 2023

Artonus May 17, 2023

Artonus May 17, 2023

Artonus May 18, 2023

Kapim May 18, 2023

Kapim May 18, 2023

ZdenekM May 19, 2023

ZdenekM May 19, 2023

ZdenekM May 19, 2023 •

edited

Loading

Artonus May 18, 2023

Kapim May 18, 2023

ZdenekM May 19, 2023

Kapim left a comment

Kapim May 18, 2023

ZdenekM May 19, 2023

Kapim May 18, 2023

ZdenekM May 19, 2023

Kapim May 18, 2023

Kapim May 18, 2023

Kapim May 18, 2023

Kapim May 18, 2023

ZdenekM commented May 19, 2023

ZdenekM commented May 19, 2023

Kapim commented Sep 20, 2023

ZdenekM commented Apr 23, 2024

feat: added method for returning full plan object #34

feat: added method for returning full plan object #34

Conversation

adrianLIrobotics commented May 12, 2023

Kapim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianLIrobotics commented May 17, 2023

Artonus 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

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

ZdenekM May 19, 2023 • edited 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

Kapim 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZdenekM commented May 19, 2023

ZdenekM commented May 19, 2023

Kapim commented Sep 20, 2023

ZdenekM commented Apr 23, 2024

ZdenekM May 19, 2023 •

edited

Loading