Conversation
|
/lgtm |
JoeyHwong-gk
force-pushed
the
devel
branch
3 times, most recently
from
ad6e4a5 to
8520bfb
Compare
Signed-off-by: JoeyHwong-gk <joeyhwong@gknow.cn>
WalkthroughThe changes update the RoboSDK documentation by adding structured README files and proposal documents covering project overview, architecture, comparisons, research proposals, roadmap, and value description. They improve backend type checking by switching to Changes
Sequence Diagram(s)sequenceDiagram
participant Auth as CloudAuthProxy
participant Event as Registered CloudAPIProxy
participant Server as Cloud Server
Auth ->> Server: Request token update (_update_token)
Server -->> Auth: Return new token & project ID
Auth ->> Event: update_token(new token, new project ID)
loop Periodic Update
Auth ->> Auth: Wait for next interval
end
sequenceDiagram
participant R as Robot Instance
participant D as Control Driver
R ->> D: Instantiate control driver
D ->> D: Initialize driver
R ->> D: connect() (ensures driver connection)
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 30
Files ignored due to filter (2)
- docs/source/_static/architecture.png
- docs/source/proposals/research/AStar path planning algorithm/animation.gif
Files selected for processing (20)
- README.md (3 hunks)
- docs/source/proposals/README.md (1 hunks)
- docs/source/proposals/comparison.md (1 hunks)
- docs/source/proposals/introduction.md (1 hunks)
- docs/source/proposals/project_design.md (1 hunks)
- docs/source/proposals/references.md (1 hunks)
- docs/source/proposals/research/AStar path planning algorithm/README.md (1 hunks)
- docs/source/proposals/research/README.md (1 hunks)
- docs/source/proposals/roadmap.md (1 hunks)
- docs/source/proposals/value_description.md (1 hunks)
- robosdk/backend/ros1.py (1 hunks)
- robosdk/backend/ros2.py (2 hunks)
- robosdk/cloud_robotics/roboartisan/base.py (1 hunks)
- robosdk/cloud_robotics/roboartisan/oms.py (1 hunks)
- robosdk/common/constant.py (2 hunks)
- robosdk/common/fileops.py (2 hunks)
- robosdk/control/legged/ysc_control.py (3 hunks)
- robosdk/core/robot.py (1 hunks)
- robosdk/utils/request.py (3 hunks)
- robosdk/utils/util.py (2 hunks)
Files skipped from review due to trivial changes (8)
- README.md
- docs/source/proposals/README.md
- docs/source/proposals/references.md
- docs/source/proposals/research/AStar path planning algorithm/README.md
- docs/source/proposals/research/README.md
- docs/source/proposals/roadmap.md
- docs/source/proposals/value_description.md
- robosdk/backend/ros2.py
Additional comments (Suppressed): 15
docs/source/proposals/comparison.md (1)
- 1-64: The comparison between RoboSDK and other existing solutions is well-detailed and comprehensive. It clearly highlights the unique features and advantages of RoboSDK over NVIDIA Isaac SDK, AWS IoT SDK, and rospy. The document provides a good understanding of why developers might prefer RoboSDK for their cloud-native robotics applications.
robosdk/backend/ros1.py (1)
- 296-302: The change from
type()toisinstance()is a good practice as it allows for subclassing. This change will not break the existing functionality and improves the flexibility of the code.- elif type(field_value) == str: + elif isinstance(field_value, str):robosdk/cloud_robotics/roboartisan/base.py (2)
165-203: The
get_tokenmethod sends a POST request containing sensitive user credentials. Ensure that this communication is happening over HTTPS to prevent potential interception of these credentials.205-236: The
get_project_idmethod retrieves the project ID from the Huawei Cloud API. Similar to theget_tokenmethod, ensure that this communication is secure.docs/source/proposals/project_design.md (2)
1-123: The documentation is well-written and provides a comprehensive overview of the project's design, architecture, and usage. It explains the core pillars of the architecture, how to instantiate and control a robot using
RoboSDK, and how to integrate with cloud services. The examples provided are clear and easy to understand. However, there are a few minor issues that need to be addressed.88-88: The sentence is incomplete. Consider revising for clarity.
- A sample is provided below, more details by visiting `robosdk::common::class_factory`. + A sample is provided below. For more details, visit `robosdk::common::class_factory`.robosdk/control/legged/ysc_control.py (3)
89-98: The
start_force_modeandstart_auto_modemethods have been changed to usesendSimpleinstead ofsendSimpleCommand. Ensure that the robot controller can handle these commands correctly. Also, verify if the arguments passed tosendSimple(3 and 4) are correct.124-133: The
_GAIT_CODEdictionary has been updated with new gait types. Make sure these changes align with the robot's capabilities and the rest of your codebase. The removal of some gait types (e.g.,GaitType.LIEON,GaitType.STAND,GaitType.HOLD,GaitType.FALL) might affect other parts of your application.166-172: New methods
force_controlandlie_downhave been added. These methods call functions onself.commander. Ensure thatself.commanderis properly initialized before these methods are called to avoid potentialAttributeError.robosdk/common/constant.py (2)
134-141: There are new gait types added to the enumeration. Ensure that these new types are handled correctly in the code where this enumeration is used.
149-166: The new
RoboArtisanenum andRoboArtisanCloudAPIdictionary have been introduced. Make sure that these new elements are integrated properly into the rest of the system, especially where cloud API interactions occur.robosdk/common/fileops.py (1)
- 53-59: The addition of
_SUB_DOMAINand_DOMAIN_NAMEfromBaseConfigis a good practice for centralizing configuration values. However, ensure that these new configurations are properly set in the environment where this code will run.robosdk/utils/request.py (1)
- 106-133: The
auth_with_iammethod has been significantly changed. The old version used access key (ak) and secret key (sk) for authentication, while the new version uses username and password. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and that the change in authentication method does not introduce security vulnerabilities.robosdk/utils/util.py (2)
14-27: The new imports
hashlibandhmacare used for generating HMAC256, MD5 hashes, and tokens. Ensure these libraries are included in the project dependencies.34-41: The
HMAC256function generates a HMAC256 hash using a key and message. This is a common method for ensuring data integrity and authenticity. However, it's important to ensure that the key used for hashing is securely stored and managed.
| Cloud-native robotics is a cutting-edge approach that leverages cloud computing and services to enhance the capabilities of robots. By offloading computation and data storage to the cloud, robots can access vast amounts of data, powerful machine learning algorithms, and computational resources, enabling them to perform complex tasks and make data-driven decisions. This approach revolutionizes the robotics industry, enabling robots to adapt and learn from real-world scenarios continuously. | ||
|
|
There was a problem hiding this comment.
The information in these lines is largely a repetition of the content in line 7. Consider revising to avoid redundancy and improve readability.
| - **Hardware Diversity:** Robots and sensors come in various Protocols and Drivers, making hardware integration complex and time-consuming. | ||
|
|
||
| - **Robot Operating System (ROS) Compatibility:** ROS versions and standards may differ across platforms, causing compatibility issues. | ||
|
|
||
| - **Cloud Service Integration:** Integrating cloud services often requires deep knowledge of specific APIs and complex configurations. | ||
|
|
||
| - **Across Component Integration:** To achieve complete application development, we require a common API and research platform for sharing code, data, and models across services, control, algorithms, and data processing modules. However, despite its long history and extensive capabilities, `ROS` can be challenging for new users without the necessary expertise or time to fully understand the software stack. |
There was a problem hiding this comment.
This section does a good job of outlining the challenges faced by developers in the field of robotics. However, it would be beneficial to also mention how RoboSDK addresses these challenges specifically. This will give readers a clear understanding of the value proposition of RoboSDK.
| `RoboSDK` is a middleware on top of the robot operating system (ROS). It provides a consistent set of hardware-independent mid-level APIs to control different robots. These are the concepts that will help you get started understanding the basics of `RoboSDK`. | ||
|
|
There was a problem hiding this comment.
The term "middleware" might not be familiar to all readers. Consider providing a brief explanation or definition of what middleware is in this context.
| - **ROS**: The Robot Operating System (ROS) is a set of software libraries and tools that help you build robot applications. From drivers to state-of-the-art algorithms, and with powerful developer tools, ROS has what you need for your next robotics project. Representative work includes: [ROS 1](https://wiki.ros.org/), [ROS 2](http://docs.ros.org/), and [OpenHarmony](https://www.openharmony.cn/). | ||
|
|
||
| - **RoboArtisan**: `RoboArtisan` is a series of cloud-native robotics solutions that aims to simplify and democratize the research of sophisticated robotics scenes. Representative work includes: `RoboOMS`, `RoboSS`, `RoboAI`. | ||
|
|
||
| - **RoboOMS**: The Robot Operation Management Service (RoboOMS) is a cloud-native robotics solution built on [KubeEdge](https://github.com/kubeedge/kubeedge). It provides a foundation for managing robot-cloud-edge communication, facilitating multi-robot collaboration, deploying applications, and conducting operations. | ||
|
|
||
| - **RoboSS**: The Robot Simulation Service (RoboSS) is a cloud-native robotics solution built on [O3DE](https://github.com/o3de/o3de). It provides a simulation framework for cloud-native scenarios, allowing users to simulate robot scenarios, generate data, train skills, and experiment with digital twins using open-source simulators. | ||
|
|
||
| - **RoboAI**: The Robot Artificial Intelligence Service (RoboAI) is a cloud-native robotics solution for building and deploying AI solutions. Representative work includes: [sedna](https://github.com/kubeedge/sedna). No newline at end of file |
There was a problem hiding this comment.
These definitions are helpful for understanding the components of RoboSDK. However, it would be useful to also explain how these components interact with each other within the RoboSDK framework.
| def __init__( | ||
| self, | ||
| service_name: RoboArtisan, | ||
| token: str = "", | ||
| project_id: str = "", | ||
| region: str = "default", | ||
| resource: str = "HuaweiCloud", | ||
| ): |
There was a problem hiding this comment.
The __init__ method of the CloudAPIProxy class has a default value for the resource parameter set to "HuaweiCloud". This hardcodes a specific cloud provider into the base class, which might limit the flexibility of your code if you plan to support multiple cloud providers in the future. Consider making this more generic.
| async def create_app( | ||
| self, name: str, | ||
| package_url: str, | ||
| package_type: str = "image", | ||
| package_arch: str = "arm64", | ||
| ros_version: str = "ros1_melodic" | ||
| ) -> str: | ||
| """ create app """ | ||
| url = f"{self.server_uri}/roboapps" | ||
|
|
||
| for app in await self.get_apps(name): | ||
| if app.get("name") == name: | ||
| if app.get("package_url") == package_url: | ||
| return app.get("id") | ||
| await self.delete_app(app.get("id")) | ||
|
|
||
| data = { | ||
| "name": name, | ||
| "package_arch": package_arch, | ||
| "package_url": package_url, | ||
| "package_type": package_type, | ||
| "robo_suite": { | ||
| "ros_version": ros_version | ||
| }, | ||
| "tags": {} | ||
| } | ||
|
|
||
| resp = await self.__session__.post( | ||
| url, | ||
| json=data, | ||
| headers=self._ext_header | ||
| ) | ||
| if resp.status != 201: | ||
| _text = await resp.text() | ||
| self.logger.debug(f"Call {url} fail: {data} => {_text}") | ||
| raise CloudError("create app failed", resp.status) | ||
|
|
||
| res = await resp.json() | ||
| return res.get("id") | ||
|
|
There was a problem hiding this comment.
In the create_app method, if an app with the same name exists but has a different package URL, the existing app is deleted and a new one is created. This could lead to data loss. Consider updating the existing app instead.
| async def get_app_deployment(self, offset: int = 0) -> List: | ||
| """ get app deployment """ | ||
| url = f"{self.server_uri}/deployments" | ||
|
|
||
| resp = await self.__session__.get( | ||
| url, | ||
| headers=self._ext_header | ||
| ) | ||
| if resp.status != 200: | ||
| _text = await resp.text() | ||
| self.logger.debug(f"Call {url} fail: => {_text}") | ||
| raise CloudError("get app deployment failed", resp.status) | ||
|
|
||
| res = await resp.json() | ||
| all_deployment = res.get("deployment_infos", []) | ||
| count = res.get("count", 0) | ||
| if count > offset + self._page_size: | ||
| all_deployment.extend( | ||
| await self.get_app_deployment( | ||
| offset=offset + self._page_size | ||
| ) | ||
| ) | ||
| return all_deployment |
There was a problem hiding this comment.
Similar to get_robots and get_apps, the get_app_deployment method uses recursion which could lead to a stack overflow if there are many pages of deployments to fetch. Consider using a while loop instead.
| async def create_app_deployment( | ||
| self, | ||
| app_id: str, | ||
| robot_id: str, | ||
| version: str = "latest", | ||
| resources=None, | ||
| command: str = "", | ||
| run_args: List = None, | ||
| run_env: Dict = None, | ||
| volumes: List = None, | ||
| additional_properties: str = "" | ||
| ): | ||
| deploy_name = genearteMD5(f"{app_id}_{version}_{robot_id}") | ||
|
|
||
| for deployment in self._deployment_data: | ||
| if deployment.get("name", "") == deploy_name: | ||
| status = deployment.get("status", "") | ||
| self.logger.debug( | ||
| f"deployment {deploy_name} already exists, {status}" | ||
| ) | ||
| await self.delete_app_deployment(deployment.get("id", "")) | ||
| launch_config = { | ||
| "host_network": True, | ||
| "privileged": False, | ||
| "additionalProperties": additional_properties, | ||
| } | ||
| if command: | ||
| launch_config["command"] = command | ||
| if volumes and isinstance(volumes, list): | ||
| launch_config["volumes"] = volumes | ||
| if run_env and isinstance(run_env, dict): | ||
| launch_config["envs"] = run_env | ||
| if run_args and isinstance(run_args, list): | ||
| launch_config["args"] = run_args | ||
| if resources and isinstance(resources, dict): | ||
| r_limits = resources.get("limits", {}) | ||
| r_requests = resources.get("requests", {}) | ||
| resources = {} | ||
| if r_limits: | ||
| resources["limits"] = r_limits | ||
| if r_requests: | ||
| resources["requests"] = r_requests | ||
| if resources: | ||
| launch_config["resources"] = resources | ||
|
|
||
| url = f"{self.server_uri}/deployments" | ||
|
|
||
| data = { | ||
| "name": deploy_name, | ||
| "robot_id": robot_id, | ||
| "description": "Deploy by teleop server, do not delete it", | ||
| "robot_app_config": { | ||
| "robot_app_id": app_id, | ||
| "version": version, | ||
| "launch_config": launch_config | ||
| } | ||
| } | ||
| resp = await self.__session__.post( | ||
| url, | ||
| json={"deployment": data}, | ||
| headers=self._ext_header | ||
| ) | ||
| if resp.status != 201: | ||
| _text = await resp.text() | ||
| self.logger.debug(f"Call {url} fail: {data} => {_text}") | ||
| raise CloudError("create app deployment failed", resp.status) | ||
| res = await resp.json() | ||
| return res.get("id", "") |
There was a problem hiding this comment.
In the create_app_deployment method, if a deployment with the same name exists, it is deleted and a new one is created. This could lead to data loss. Consider updating the existing deployment instead.
| def genearteMD5(item) -> str: | ||
| hl = hashlib.md5() | ||
| hl.update(item.encode(encoding='utf-8')) | ||
| return hl.hexdigest() |
There was a problem hiding this comment.
The generateMD5 function generates an MD5 hash of the input item. Be aware that MD5 is considered cryptographically broken and unsuitable for further use. Its main weakness is that it does not have strong collision resistance, and thus it is no longer suitable as a cryptographic hash function for SSL certificates, or as a checksum to verify data against tampering. Consider using a more secure hash function if this is a concern.
- def genearteMD5(item) -> str:
+ def generateSHA256(item) -> str:
- hl = hashlib.md5()
+ hl = hashlib.sha256()
hl.update(item.encode(encoding='utf-8'))
return hl.hexdigest()| def gen_token(length: int = 4): | ||
| """ | ||
| generate token | ||
| """ | ||
| return str( | ||
| hashlib.sha256(str(datetime.now()).encode('utf-8')).hexdigest() | ||
| )[:length] |
There was a problem hiding this comment.
The gen_token function generates a token by hashing the current datetime and taking the first length characters. This approach may not generate sufficiently random tokens, especially if length is small. Consider using a more robust method for token generation, such as secrets.token_hex().
- def gen_token(length: int = 4):
+ def gen_token(length: int = 16):
- return str(
- hashlib.sha256(str(datetime.now()).encode('utf-8')).hexdigest()
- )[:length]
+ import secrets
+ return secrets.token_hex(length)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
robosdk/core/robot.py (1)
209-210: 🛠️ Refactor suggestionAdd error handling around driver connection
While adding the driver.connect() call is a good fix, it doesn't include error handling to verify if the connection was successful.
- driver.connect() - setattr(self, ctl.lower(), driver) + if driver.connect(): + setattr(self, ctl.lower(), driver) + else: + self.logger.error(f"Failed to connect to {ctl}")Based on the backend implementations in other files, the connect() method should return a success indicator or set a has_connect flag that could be checked.
🧹 Nitpick comments (15)
docs/source/proposals/research/AStar path planning algorithm/README.md (1)
7-7: Fix grammar issue with determinersThere's a small grammar issue with "The A* algorithm" containing two determiners in a row.
-The A* algorithm is a popular path planning algorithm widely used in robotics and artificial intelligence applications. +A* algorithm is a popular path planning algorithm widely used in robotics and artificial intelligence applications.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Spaces inside emphasis markers
null(MD037, no-space-in-emphasis)
robosdk/common/fileops.py (1)
361-364: Enhance region configuration with validation and logging.The code now supports region-specific IAM server URLs, which is a good improvement. However, the hardcoded default region "cn-south-1" might not be appropriate for all deployments.
- region = ctx.get(cls._DOMAIN_NAME, "cn-south-1").strip() + region = ctx.get("REGION", ctx.get(cls._DOMAIN_NAME, "cn-south-1")).strip() + if not region or not isinstance(region, str): + region = "cn-south-1" + logging.warning(f"Invalid region specified, defaulting to {region}")docs/source/proposals/roadmap.md (1)
1-16: LGTM with minor grammatical suggestion.The roadmap provides a clear overview of the project's development plan and completed milestones. Consider hyphenating "high-level" as it's a compound adjective modifying "roadmap".
- This document defines a high level roadmap for RoboSDK development. + This document defines a high-level roadmap for RoboSDK development.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: # Roadmap This document defines a high level roadmap for RoboSDK development. The [...(EN_COMPOUND_ADJECTIVE_INTERNAL)
robosdk/utils/util.py (1)
34-41: Improve HMAC256 function documentation.The function documentation is minimal and doesn't explain the purpose or security considerations of HMAC SHA-256.
def HMAC256(key: str, msg: str) -> str: """ - HMAC256 + Compute HMAC using SHA-256 for message authentication. + + Args: + key (str): The secret key for the HMAC + msg (str): The message to authenticate + + Returns: + str: The hexadecimal digest of the HMAC """ return hmac.new( bytes(key, 'utf-8'), bytes(msg, 'utf-8'), hashlib.sha256 ).hexdigest()robosdk/common/constant.py (1)
159-166: Consider a more flexible cloud API structure.The current implementation hardcodes URLs to Huawei Cloud, which might limit flexibility if other cloud providers need to be supported in the future. Consider implementing a more provider-agnostic approach.
You could refactor this to a class-based structure that supports multiple providers:
-RoboArtisanCloudAPI = { - "HuaweiCloud": { - RoboArtisan.iam: "https://iam.{region}.myhuaweicloud.com/v3", - RoboArtisan.robooms: "https://oms.{region}.myhuaweicloud.com/v1", - RoboArtisan.roboss: "https://ss.{region}.myhuaweicloud.com/v1", - RoboArtisan.robomap: "https://map.{region}.myhuaweicloud.com/v1" - } -} +class CloudProvider(Enum): + """Cloud providers supported by RoboArtisan""" + HUAWEI_CLOUD = "HuaweiCloud" + +class RoboArtisanCloudAPI: + """API endpoints for RoboArtisan services across cloud providers""" + + _ENDPOINTS = { + CloudProvider.HUAWEI_CLOUD: { + RoboArtisan.iam: "https://iam.{region}.myhuaweicloud.com/v3", + RoboArtisan.robooms: "https://oms.{region}.myhuaweicloud.com/v1", + RoboArtisan.roboss: "https://ss.{region}.myhuaweicloud.com/v1", + RoboArtisan.robomap: "https://map.{region}.myhuaweicloud.com/v1" + } + } + + @classmethod + def get_endpoint(cls, provider: CloudProvider, service: RoboArtisan, region: str) -> str: + """Get the endpoint URL for a specific service and region.""" + if provider not in cls._ENDPOINTS or service not in cls._ENDPOINTS[provider]: + raise ValueError(f"Unsupported provider or service: {provider.value}/{service.value}") + return cls._ENDPOINTS[provider][service].format(region=region)docs/source/proposals/project_design.md (1)
13-13: Use a hyphen for "hardware-agnostic".In compound adjectives, a hyphen is typically used.
Here's the suggested diff:
- making it easier to create robot applications that are hardware agnostic. + making it easier to create robot applications that are hardware-agnostic.🧰 Tools
🪛 LanguageTool
[misspelling] ~13-~13: This word is normally spelled with a hyphen.
Context: ...r to create robot applications that are hardware agnostic. Whether you are working with different...(EN_COMPOUNDS_HARDWARE_AGNOSTIC)
docs/source/proposals/value_description.md (1)
9-9: Correct the misspelling “Simmulation”.Apply this diff to rectify the typo:
- tap into AI, Simmulation capabilities, + tap into AI, Simulation capabilities,README.md (2)
26-26: Fix subject-verb agreement.Change “runs” to “run” when referring to “APPs”:
- The goal of this project is to abstract away the low-level controls for developing APPs that runs on robots + The goal of this project is to abstract away the low-level controls for developing APPs that run on robots🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...level controls for developing APPs that runs on robots in an easy-to-use way. ### F...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
172-172: Use a more professional phrasing instead of "feel free to".An alternative phrasing example:
- Feel free to reach out to us + We invite you to reach out to us🧰 Tools
🪛 LanguageTool
[style] ~172-~172: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...ollaborate with like-minded developers. Feel free to reach out to us through [this documents...(FEEL_FREE_TO_STYLE_ME)
robosdk/cloud_robotics/roboartisan/oms.py (3)
22-22: Minor naming inconsistency in imported function.
The function namegenearteMD5appears to be misspelled. Consider renaming it in the utility module to avoid confusion.
50-64: Consider refining the indefinite loop approach.
Therunmethod continuously polls data in a tight loop. This could block shutdown ifclose()is not called and might be expensive on large-scale deployments. Evaluate adding a more graceful scheduling or backoff mechanism.
65-76: Concurrent access to data outside locks.
Reading properties likerobotsanddeploymentsreturns mutable structures without acquiring the corresponding locks. This can lead to race conditions if the data is accessed or modified elsewhere concurrently.docs/source/proposals/comparison.md (3)
19-23: Well-Articulated Advantages for NVIDIA Comparison
The bullet list neatly summarizes RoboSDK’s benefits, particularly its generic hardware abstraction layer and flexible middleware integration.
Note: Consider reviewing the phrasing in this header for possible repetitive wording, as hinted by the static analysis at line ~19, to ensure maximum clarity.🧰 Tools
🪛 LanguageTool
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...needs of developers. ### Advantages of RoboSDK - RoboSDK's generic hardware abstraction layer en...(ENGLISH_WORD_REPEAT_RULE)
38-42: Reinforcing RoboSDK Advantages for AWS IoT SDK Comparison
The bullet points in this section reiterate RoboSDK's strengths clearly.
Note: Consider rephrasing the "Advantages of RoboSDK" header to avoid potential repetition, as noted by static analysis in line ~38.🧰 Tools
🪛 LanguageTool
[duplication] ~38-~38: Possible typo: you repeated a word.
Context: ...n's cloud resources. ### Advantages of RoboSDK - RoboSDK's specialized focus on cloud-native rob...(ENGLISH_WORD_REPEAT_RULE)
57-61: Summary of Advantages in the rospy Comparison with a Minor Suggestion
The bullet points effectively summarize the benefits that RoboSDK brings over rospy.
Note: Similar to earlier sections, consider revisiting the "Advantages of RoboSDK" header wording to minimize repetition (as suggested by the static analysis for line ~57). This slight rephrasing could enhance readability.🧰 Tools
🪛 LanguageTool
[duplication] ~57-~57: Possible typo: you repeated a word.
Context: ...ying robot platform. ### Advantages of RoboSDK - RoboSDK's higher-level interface abstracts away...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/source/_static/architecture.pngis excluded by!**/*.pngdocs/source/proposals/research/AStar path planning algorithm/animation.gifis excluded by!**/*.gif
📒 Files selected for processing (20)
README.md(3 hunks)docs/source/proposals/README.md(1 hunks)docs/source/proposals/comparison.md(1 hunks)docs/source/proposals/introduction.md(1 hunks)docs/source/proposals/project_design.md(1 hunks)docs/source/proposals/references.md(1 hunks)docs/source/proposals/research/AStar path planning algorithm/README.md(1 hunks)docs/source/proposals/research/README.md(1 hunks)docs/source/proposals/roadmap.md(1 hunks)docs/source/proposals/value_description.md(1 hunks)robosdk/backend/ros1.py(1 hunks)robosdk/backend/ros2.py(2 hunks)robosdk/cloud_robotics/roboartisan/base.py(1 hunks)robosdk/cloud_robotics/roboartisan/oms.py(1 hunks)robosdk/common/constant.py(2 hunks)robosdk/common/fileops.py(2 hunks)robosdk/control/legged/ysc_control.py(3 hunks)robosdk/core/robot.py(1 hunks)robosdk/utils/request.py(3 hunks)robosdk/utils/util.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
robosdk/core/robot.py (3)
robosdk/backend/ros2.py (1)
connect(52-74)robosdk/backend/ros1.py (1)
connect(64-90)robosdk/backend/base.py (1)
connect(36-37)
robosdk/common/fileops.py (2)
robosdk/common/config.py (2)
BaseConfig(251-312)get(242-247)robosdk/utils/request.py (1)
auth_with_iam(105-142)
robosdk/utils/request.py (1)
robosdk/cloud_robotics/roboartisan/base.py (3)
get_project_id(205-236)token(114-115)project_id(118-119)
robosdk/control/legged/ysc_control.py (1)
robosdk/common/constant.py (1)
GaitType(132-141)
robosdk/cloud_robotics/roboartisan/oms.py (3)
robosdk/cloud_robotics/roboartisan/base.py (8)
CloudAPIProxy(28-79)update_token(62-63)update_token(103-111)token(114-115)project_id(118-119)run(59-60)run(121-134)server_uri(70-76)robosdk/common/constant.py (1)
RoboArtisan(149-156)robosdk/utils/util.py (3)
genearteMD5(44-47)get(159-164)update(155-156)
🪛 LanguageTool
docs/source/proposals/research/AStar path planning algorithm/README.md
[grammar] ~6-~6: Two determiners in a row. Choose either “The” or “A”.
Context: ...obotics applications. ## Introduction The A* algorithm is a popular path planning a...
(DT_DT)
docs/source/proposals/roadmap.md
[uncategorized] ~3-~3: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: # Roadmap This document defines a high level roadmap for RoboSDK development. The [...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/source/proposals/README.md
[uncategorized] ~9-~9: A determiner appears to be missing. Consider inserting it.
Context: ... Here provides a proposal to RoboSDK, comprises the following key sections: 1. **Intro...
(AI_EN_LECTOR_MISSING_DETERMINER)
docs/source/proposals/comparison.md
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...needs of developers. ### Advantages of RoboSDK - RoboSDK's generic hardware abstraction layer en...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~38-~38: Possible typo: you repeated a word.
Context: ...n's cloud resources. ### Advantages of RoboSDK - RoboSDK's specialized focus on cloud-native rob...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~57-~57: Possible typo: you repeated a word.
Context: ...ying robot platform. ### Advantages of RoboSDK - RoboSDK's higher-level interface abstracts away...
(ENGLISH_WORD_REPEAT_RULE)
docs/source/proposals/introduction.md
[style] ~19-~19: Consider an alternative adjective to strengthen your wording.
Context: ...tegrating cloud services often requires deep knowledge of specific APIs and complex ...
(DEEP_PROFOUND)
README.md
[uncategorized] ~26-~26: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...level controls for developing APPs that runs on robots in an easy-to-use way. ### F...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[style] ~172-~172: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...ollaborate with like-minded developers. Feel free to reach out to us through [this documents...
(FEEL_FREE_TO_STYLE_ME)
docs/source/proposals/project_design.md
[misspelling] ~13-~13: This word is normally spelled with a hyphen.
Context: ...r to create robot applications that are hardware agnostic. Whether you are working with different...
(EN_COMPOUNDS_HARDWARE_AGNOSTIC)
[uncategorized] ~15-~15: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...h the imperative programming, following provide a simple example for understanding, you...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 markdownlint-cli2 (0.17.2)
docs/source/proposals/research/AStar path planning algorithm/README.md
7-7: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
docs/source/proposals/project_design.md
90-90: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🔇 Additional comments (42)
robosdk/backend/ros1.py (1)
299-299: Good improvement in type checking!Using
isinstance()instead oftype() ==is a better practice as it properly handles inheritance and is the Python-recommended way to check types. This change will ensure that subclasses ofstrwill also be correctly identified.robosdk/backend/ros2.py (2)
209-209: Good improvement in type checking!Using
isinstance()instead oftype() ==is a better practice as it properly handles inheritance and is the Python-recommended way to check types. This is consistent with the same change made in ros1.py.
264-264: Good improvement in type checking!Using
isinstance()instead of direct type comparison in the ROS array conversion logic is a good practice. This maintains consistency with other type checks and properly handles potential string subclasses.docs/source/proposals/research/README.md (1)
1-3: Clearly defines the purpose of the research directoryThe README provides clear expectations about the purpose of the research directory, emphasizing that it contains valuable research efforts but doesn't guarantee immediate implementation.
docs/source/proposals/research/AStar path planning algorithm/README.md (2)
11-20: Well-structured research objectivesThe objectives section clearly outlines the goals of integrating the A* algorithm into RoboSDK with a focus on performance analysis and cloud-native applications.
9-9:Details
✅ Verification successful
Verify that the referenced image file exists
The README references an animation file that should be included with the documentation.
🏁 Script executed:
#!/bin/bash # Check if the animation.gif file exists in the directory fd -p "docs/source/proposals/research/AStar path planning algorithm/animation.gif" -t fLength of output: 164
Verified: Referenced Image Exists
The animation file is verified in the expected directory (
docs/source/proposals/research/AStar path planning algorithm/animation.gif). No changes are needed.docs/source/proposals/references.md (1)
1-9: References structure is in place with placeholdersThe references document provides a good structure with placeholders for future content. This is adequate for now, though you'll want to populate these sections with actual content in the future.
robosdk/common/fileops.py (2)
56-56: LGTM: Adding a new configuration parameter for sub-domain.The addition of
_SUB_DOMAINproperly leverages the configuration from BaseConfig, allowing more flexibility in the file transfer configuration.
367-370: Improve exception handling during authentication.The exception handling is too broad and silently ignores all errors, which could hide critical issues.
try: client.auth_with_iam( _ak, _sk, server=iam_server, domain=domain, project_id=region) - except: # noqa - pass + except Exception as e: # replace Exception with specific exceptions if possible + logging.error(f"Error during authentication: {e}")🧰 Tools
🪛 Ruff (0.8.2)
365-370: Use
contextlib.suppress(Exception)instead oftry-except-pass(SIM105)
robosdk/utils/util.py (2)
44-47: Fix typo in function name and consider using a more secure hash.The function name has a typographical error and uses MD5, which is cryptographically broken and unsuitable for security-sensitive use cases.
- def genearteMD5(item) -> str: + def generateSHA256(item) -> str: - hl = hashlib.md5() + hl = hashlib.sha256() hl.update(item.encode(encoding='utf-8')) return hl.hexdigest()
50-56: Use cryptographically secure token generation.The current implementation uses a datetime-based approach, which is not cryptographically secure and may be predictable.
- def gen_token(length: int = 4): + def gen_token(length: int = 16): """ - generate token + Generate a cryptographically secure random token. + + Args: + length (int): The number of bytes to generate (hex output will be 2*length characters) + + Returns: + str: A secure random token """ - return str( - hashlib.sha256(str(datetime.now()).encode('utf-8')).hexdigest() - )[:length] + import secrets + return secrets.token_hex(length)robosdk/common/constant.py (3)
137-138: LGTM: Expanding the GaitType enum with new values.The addition of
RUNandSLIPgait types enhances the functionality for legged robot control systems.
141-141: Fix typographical error in enum value name.There is a typographical error in the enum value name. It should be
UNKNOWNinstead ofUNKONWN.- UNKONWN = 99 + UNKNOWN = 99
149-157: LGTM: New RoboArtisan enum for meta services.The RoboArtisan enum provides a clear structure for the different services available in the framework.
docs/source/proposals/introduction.md (1)
9-10: Remove redundant information.The information in these lines largely repeats the content in line 7. Consider revising to eliminate redundancy and improve readability.
-Cloud-native robotics is a cutting-edge approach that leverages cloud computing and services to enhance the capabilities of robots. By offloading computation and data storage to the cloud, robots can access vast amounts of data, powerful machine learning algorithms, and computational resources, enabling them to perform complex tasks and make data-driven decisions. This approach revolutionizes the robotics industry, enabling robots to adapt and learn from real-world scenarios continuously.robosdk/utils/request.py (1)
208-208: Move import statement to the top of the file.The import statement for
aiofilesis inside theasync_downloadmethod. It's a best practice to keep all imports at the top of the file unless there's a specific reason not to do so.- import aiofiles # noqaAnd add this import at the top of the file (around line 20):
+ import aiofilesrobosdk/control/legged/ysc_control.py (1)
126-131:Details
❓ Verification inconclusive
Significant change to gait mappings may affect functionality.
The
_GAIT_CODEdictionary has been completely changed, removing mappings forLIEON,STAND,HOLD,FALLand replacing them with different mappings. This could break existing code that relies on the previous mappings.Verify that all existing functionality that depends on these gait mappings has been updated accordingly. Consider whether you need to maintain backward compatibility or document this as a breaking change.
🏁 Script executed:
#!/bin/bash # Search for places where the old gait types might be used echo "Searching for potential references to removed gait types:" rg -A 5 "GaitType\.LIEON|GaitType\.STAND|GaitType\.HOLD|GaitType\.FALL" --type pyLength of output: 208
Manual Verification Required: Confirm Gait Mapping Consistency
The updated
_GAIT_CODEinrobosdk/control/legged/ysc_control.py(lines 126–131) no longer includesLIEON,STAND,HOLD, andFALL. A regex search for these removed gait types yielded no matches; however, the absence of output might not cover all cases (e.g., external modules or tests). Please manually verify that all functionality depending on the old mappings has been updated accordingly, and confirm whether backward compatibility should be maintained or this change documented as breaking.docs/source/proposals/project_design.md (1)
15-15: Incorporate the previously suggested grammatical fix.🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...h the imperative programming, following provide a simple example for understanding, you...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
robosdk/cloud_robotics/roboartisan/base.py (5)
40-40: Consider making theresourceparameter more generic.
54-54: Re-enable SSL verification for security.
70-76: Consider a more robust approach to building theserver_uri.
145-150: Use secure handling for credentials instead of retrieving them directly from config.
160-162: Catching broad exceptions can mask specific errors.robosdk/cloud_robotics/roboartisan/oms.py (8)
39-39: Unused lock variable.
The_app_data_lockis declared but never used. If it's not needed, consider removing it for clarity; otherwise, ensure it is utilized consistently for thread safety.
43-49: Token and project_id validation concern.
Previous reviews suggested raising an exception if eithertokenorproject_idis mandatory but not provided. Currently, missing values won’t immediately cause an error, which could be intentional or an oversight.
77-89: Handling unknown robot types.
Ifrobot_typeis not found in_all_properties_map, you return an empty dictionary. Past reviews noted that an unset or missing type could be unexpected and lead to incomplete data. Confirm that this behavior is intended.
90-125: Recursive fetching might risk high recursion depth.
Like past reviews observed, callingself.get_robots(...)recursively for pagination could cause stack issues with very large datasets. A loop-based pagination approach is safer.
186-225: Deleting existing apps with different package URLs can cause data loss.
As previously discussed, removing the existing app instead of updating it could be risky. Confirm that this destructive behavior is acceptable for your use case.
253-286: Replacing existing app versions.
Similar to app creation, deleting an existing version before creating a new one might lead to data loss if that version is still in use.
298-321: Recursive fetching for get_app_deployment.
Again, a large result set may cause deep recursion. An iterative solution will be more robust.
403-417: Potentially unhandled status for get_app_deployment_status.
The method defaults to"failure"ifstatusis missing, but that may mask more nuanced states. Verify that the API always returns a status or confirm the fallback is acceptable.docs/source/proposals/comparison.md (11)
1-3: Strong Introduction and Title
The document title and opening lines clearly define the scope and purpose of the comparison. The introduction is engaging and provides useful context about the frameworks being compared.
5-10: Comprehensive Overview of NVIDIA Isaac SDK
The section on NVIDIA Isaac SDK is well-structured and clearly contrasts its specialized hardware abstraction with RoboSDK’s more flexible approach. The use of bullet points aids readability.
11-14: Clear Discussion of Cloud Service Integration for NVIDIA Isaac SDK
The bullet points effectively distinguish NVIDIA’s focus on proprietary cloud services from RoboSDK’s middleware-based strategy for broader cloud integration. The explanation is concise.
15-18: Effective Comparison on Support for Robot Operating Systems
This section successfully communicates the differences between NVIDIA’s ROS 2 specialization and RoboSDK’s platform-agnostic support. The content is clear and benefits the target audience.
24-29: Concise Overview of AWS IoT SDK Characteristics
The section introduces AWS IoT SDK effectively and outlines its general approach to IoT. The contrast with RoboSDK’s robotics-specific features is clear and informative.
30-33: Clear Explanation of Robot-Specific Abstraction
The content here provides a solid explanation on how AWS IoT SDK lacks the specialized abstraction that RoboSDK offers. The bullet points are succinct and relevant.
34-37: Insightful Details on Middleware Integration
The points regarding middleware for RoboArtisan Cloud Services are very informative and effectively illustrate the added value of RoboSDK’s approach compared to AWS IoT SDK.
43-48: Effective Introduction to rospy Comparison
The introduction of the rospy section and its focus on a ROS-centric approach sets the stage well for a comparison with RoboSDK. The contrast regarding ease-of-use and abstraction levels is clear.
49-52: Concise Discussion of Cloud Service Integration in rospy
The explanation highlighting rospy’s lack of native cloud service integration compared to RoboSDK’s built-in middleware is well-presented.
53-56: Clarification on Hardware Abstraction for rospy
This section clearly distinguishes the absence of a dedicated hardware abstraction layer in rospy, contrasting it with the more comprehensive approach of RoboSDK.
62-64: Strong Conclusion That Recaps the Comparison
The conclusion efficiently summarizes the key differences and advantages of RoboSDK, reinforcing its positioning as a cloud-native robotics framework. The closing remarks are persuasive and well-aligned with the document’s intent.
|
|
||
| `RoboSDK` aims to address these challenges and provide developers with a unified platform for cloud-native robotics development. | ||
|
|
||
| Here provides a proposal to `RoboSDK`, comprises the following key sections: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Grammar needs improvement in this introduction.
The sentence structure is awkward. The phrase "Here provides" is incorrect, and there's a missing determiner.
-Here provides a proposal to `RoboSDK`, comprises the following key sections:
+Here we provide a proposal for `RoboSDK`, which comprises the following key sections:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Here provides a proposal to `RoboSDK`, comprises the following key sections: | |
| Here we provide a proposal for `RoboSDK`, which comprises the following key sections: |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: A determiner appears to be missing. Consider inserting it.
Context: ... Here provides a proposal to RoboSDK, comprises the following key sections: 1. **Intro...
(AI_EN_LECTOR_MISSING_DETERMINER)
| def get_project_id( | ||
| self, server: str, token: str, region: str = "cn-south-1") -> str: | ||
| """ get project id from huawei cloud api """ | ||
| if not (token and server): | ||
| return region | ||
| _headers = { | ||
| "Content-Type": "application/json;charset=utf8", | ||
| "X-Auth-Token": token | ||
| } | ||
| resp = self._loop.run_until_complete( | ||
| self._client.get( | ||
| url=server, | ||
| headers=_headers, | ||
| params={ | ||
| "enabled": True, | ||
| "name": region | ||
| }, | ||
| ) | ||
| ) | ||
| project = resp.json().get("projects", []) | ||
| if not project: | ||
| return region | ||
| return project[0]["id"] |
There was a problem hiding this comment.
Improve error handling in the get_project_id method.
The method doesn't check the response status before attempting to parse JSON, which could raise exceptions if the API call fails. Also, the method returns region when it can't get a project ID, which is potentially confusing. Consider following the pattern used in robosdk/cloud_robotics/roboartisan/base.py for proper error handling.
def get_project_id(
self, server: str, token: str, region: str = "cn-south-1") -> str:
""" get project id from huawei cloud api """
if not (token and server):
- return region
+ raise ValueError("Both token and server must be provided.")
_headers = {
"Content-Type": "application/json;charset=utf8",
"X-Auth-Token": token
}
resp = self._loop.run_until_complete(
self._client.get(
url=server,
headers=_headers,
params={
"enabled": True,
"name": region
},
)
)
+ if resp.status != 200:
+ self.logger.debug(f"Call {server} failed: status code: {resp.status}")
+ raise Exception(f"API call failed, status code: {resp.status}")
project = resp.json().get("projects", [])
if not project:
- return region
+ self.logger.debug(f"No projects found for region {region}")
+ return None
return project[0]["id"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_project_id( | |
| self, server: str, token: str, region: str = "cn-south-1") -> str: | |
| """ get project id from huawei cloud api """ | |
| if not (token and server): | |
| return region | |
| _headers = { | |
| "Content-Type": "application/json;charset=utf8", | |
| "X-Auth-Token": token | |
| } | |
| resp = self._loop.run_until_complete( | |
| self._client.get( | |
| url=server, | |
| headers=_headers, | |
| params={ | |
| "enabled": True, | |
| "name": region | |
| }, | |
| ) | |
| ) | |
| project = resp.json().get("projects", []) | |
| if not project: | |
| return region | |
| return project[0]["id"] | |
| def get_project_id( | |
| self, server: str, token: str, region: str = "cn-south-1") -> str: | |
| """ get project id from huawei cloud api """ | |
| if not (token and server): | |
| raise ValueError("Both token and server must be provided.") | |
| _headers = { | |
| "Content-Type": "application/json;charset=utf8", | |
| "X-Auth-Token": token | |
| } | |
| resp = self._loop.run_until_complete( | |
| self._client.get( | |
| url=server, | |
| headers=_headers, | |
| params={ | |
| "enabled": True, | |
| "name": region | |
| }, | |
| ) | |
| ) | |
| if resp.status != 200: | |
| self.logger.debug(f"Call {server} failed: status code: {resp.status}") | |
| raise Exception(f"API call failed, status code: {resp.status}") | |
| project = resp.json().get("projects", []) | |
| if not project: | |
| self.logger.debug(f"No projects found for region {region}") | |
| return None | |
| return project[0]["id"] |
|
|
||
| def start_force_mode(self): | ||
| self.sendSimpleCommand("START_FORCE_MODE") | ||
| self.sendSimple(3) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use command code constants instead of hardcoded values.
You're using a hardcoded value 3 instead of the defined constant from _command_code. This reduces maintainability and readability of the code.
- self.sendSimple(3)
+ self.sendSimpleCommand("START_FORCE_MODE")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.sendSimple(3) | |
| self.sendSimpleCommand("START_FORCE_MODE") |
| def start_auto_mode(self): | ||
| self.sendSimple(4) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use command code constants for new method.
The method uses a hardcoded value 4 which doesn't exist in the _command_code dictionary. This creates inconsistency in how commands are handled.
First, add a new entry to the _command_code dictionary:
_command_code = {
"STAND_UP_DOWN": 1,
"START_FORCE_MODE": 2,
"MOTION_START_STOP": 3,
+ "START_AUTO_MODE": 4,
"DANCE": 19,
"CHANGE_GAIT": 25,
"HEART_BEAT": 33
}Then update the method:
def start_auto_mode(self):
- self.sendSimple(4)
+ self.sendSimpleCommand("START_AUTO_MODE")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def start_auto_mode(self): | |
| self.sendSimple(4) | |
| # Somewhere in the module where the command codes are defined: | |
| _command_code = { | |
| "STAND_UP_DOWN": 1, | |
| "START_FORCE_MODE": 2, | |
| "MOTION_START_STOP": 3, | |
| "START_AUTO_MODE": 4, | |
| "DANCE": 19, | |
| "CHANGE_GAIT": 25, | |
| "HEART_BEAT": 33 | |
| } | |
| # Later in the file, within the class definition: | |
| def start_auto_mode(self): | |
| self.sendSimpleCommand("START_AUTO_MODE") |
| async def get_apps(self, name: str = "", offset: int = 0) -> List: | ||
| """ get all app """ | ||
| url = f"{self.server_uri}/roboapps" | ||
| data = { | ||
| "name": name, | ||
| "limit": int(self._page_size), | ||
| "offset": int(offset), | ||
| "sort_key": "created_at", | ||
| "sort_dir": "desc" | ||
| } | ||
| resp = await self.__session__.get( | ||
| url, | ||
| params=data, | ||
| headers=self._ext_header | ||
| ) | ||
| if resp.status != 200: | ||
| _text = await resp.text() | ||
| self.logger.debug(f"Call {url} fail: {data} => {_text}") | ||
| raise CloudError("get oms instance failed", resp.status) | ||
|
|
||
| res = await resp.json() | ||
| all_app = res.get("roboapps", []) | ||
| count = res.get("page", {}).get("count", 0) | ||
| if count > offset + self._page_size: | ||
| all_app.extend( | ||
| await self.get_apps( | ||
| name=name, | ||
| offset=offset + self._page_size | ||
| ) | ||
| ) | ||
| return all_app | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Similar recursion pattern for get_apps.
Similar to get_robots, consider changing this recursion to an iterative approach to prevent deep recursion in large data sets.
| async def delete_app(self, app_id: str): | ||
| """ delete app """ | ||
| url = f"{self.server_uri}/roboapps/{app_id}" | ||
| try: | ||
| await self.__session__.delete( | ||
| url, | ||
| headers=self._ext_header | ||
| ) | ||
| except: # noqa | ||
| self.logger.error("delete app failed") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broad exception clause with no re-raise.
Using a bare except risks masking critical exceptions. Consider catching specific exceptions or re-raising after logging to avoid silent failures.
| async def delete_app_version(self, app_id: str, version: str): | ||
| """ delete app version """ | ||
| url = f"{self.server_uri}/roboapps/{app_id}/versions/{version}" | ||
| try: | ||
| await self.__session__.delete( | ||
| url, | ||
| headers=self._ext_header | ||
| ) | ||
| except: # noqa | ||
| self.logger.error("delete app version failed") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broad exception in delete_app_version.
Likewise, catching all exceptions may hide important errors. Consider more targeted exception handling and re-raising if necessary.
| async def create_app_deployment( | ||
| self, | ||
| app_id: str, | ||
| robot_id: str, | ||
| version: str = "latest", | ||
| resources=None, | ||
| command: str = "", | ||
| run_args: List = None, | ||
| run_env: Dict = None, | ||
| volumes: List = None, | ||
| additional_properties: str = "" | ||
| ): | ||
| deploy_name = genearteMD5(f"{app_id}_{version}_{robot_id}") | ||
|
|
||
| for deployment in self._deployment_data: | ||
| if deployment.get("name", "") == deploy_name: | ||
| status = deployment.get("status", "") | ||
| self.logger.debug( | ||
| f"deployment {deploy_name} already exists, {status}" | ||
| ) | ||
| await self.delete_app_deployment(deployment.get("id", "")) | ||
| launch_config = { | ||
| "host_network": True, | ||
| "privileged": False, | ||
| "additionalProperties": additional_properties, | ||
| } | ||
| if command: | ||
| launch_config["command"] = command | ||
| if volumes and isinstance(volumes, list): | ||
| launch_config["volumes"] = volumes | ||
| if run_env and isinstance(run_env, dict): | ||
| launch_config["envs"] = run_env | ||
| if run_args and isinstance(run_args, list): | ||
| launch_config["args"] = run_args | ||
| if resources and isinstance(resources, dict): | ||
| r_limits = resources.get("limits", {}) | ||
| r_requests = resources.get("requests", {}) | ||
| resources = {} | ||
| if r_limits: | ||
| resources["limits"] = r_limits | ||
| if r_requests: | ||
| resources["requests"] = r_requests | ||
| if resources: | ||
| launch_config["resources"] = resources | ||
|
|
||
| url = f"{self.server_uri}/deployments" | ||
|
|
||
| data = { | ||
| "name": deploy_name, | ||
| "robot_id": robot_id, | ||
| "description": "Deploy by teleop server, do not delete it", | ||
| "robot_app_config": { | ||
| "robot_app_id": app_id, | ||
| "version": version, | ||
| "launch_config": launch_config | ||
| } | ||
| } | ||
| resp = await self.__session__.post( | ||
| url, | ||
| json={"deployment": data}, | ||
| headers=self._ext_header | ||
| ) | ||
| if resp.status != 201: | ||
| _text = await resp.text() | ||
| self.logger.debug(f"Call {url} fail: {data} => {_text}") | ||
| raise CloudError("create app deployment failed", resp.status) | ||
| res = await resp.json() | ||
| return res.get("id", "") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing lock usage when mutating _deployment_data.
In create_app_deployment, the code checks and removes existing deployments from _deployment_data without acquiring _deployment_data_lock. This may lead to race conditions if run() or other methods update _deployment_data concurrently.
| async def delete_app_deployment(self, deployment_id: str): | ||
| """ delete app deployment """ | ||
| url = f"{self.server_uri}/deployments/{deployment_id}" | ||
| try: | ||
| await self.__session__.delete( | ||
| url, | ||
| headers=self._ext_header | ||
| ) | ||
| self._deployment_data = await self.get_app_deployment() | ||
| except: # noqa | ||
| self.logger.error("delete app deployment failed") | ||
|
|
There was a problem hiding this comment.
Asynchronous data update without lock protection.
delete_app_deployment also modifies _deployment_data after the deletion call, but does not use _deployment_data_lock. This could cause inconsistent or corrupted state under concurrent load.
1 participant
Description
Add proposal docs
Linked issue(s)/Pull request(s)
Type of Change
Has Been Tested
OS:
Python version:
Key information snapshot(s):
NA
Needs Follow Up Actions
Checklist
Summary by CodeRabbit
Documentation
New Features
Refactor
This release improves user resources, boosts cloud integration, and enhances overall operational efficiency.