Bump 1.0.2#90
Conversation
…ules, enhancing modularity and error handling during imports.
… to streamline package requirements.
Summary of ChangesHello @GyuH13, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces version 1.0.2, primarily focusing on enhancing the robustness and configurability of the DQN (Deep Q-Network) package. Key improvements include resolving dependency conflicts through lazy TensorFlow imports, transitioning command-line arguments to ROS parameters for greater flexibility, and refining model loading and saving mechanisms. These changes aim to streamline development, improve user experience, and prepare the package for a new ROS distro release. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements, such as version bumping to 1.0.2, lazy loading of TensorFlow to handle dependency issues, and transitioning from command-line arguments to ROS 2 parameters for configuration. These are all positive changes that improve the package's usability and robustness. My review focuses on further enhancing robustness by adding error handling for file operations and parameter validation. I've also noted a change in the model saving behavior that might have unintended consequences on disk usage.
| model_file = self.get_parameter('model_file').get_parameter_value().string_value | ||
| use_gpu = self.get_parameter('use_gpu').get_parameter_value().bool_value |
There was a problem hiding this comment.
The DQNTest node requires the model_file parameter to be set to a valid model file for testing. If this parameter is not provided, it defaults to an empty string, which will cause self.load_model() to fail with a potentially unclear error message. It would be more robust to add an explicit check for the model_file parameter and exit with an informative error if it's not provided.
| model_file = self.get_parameter('model_file').get_parameter_value().string_value | |
| use_gpu = self.get_parameter('use_gpu').get_parameter_value().bool_value | |
| model_file = self.get_parameter('model_file').get_parameter_value().string_value | |
| if not model_file: | |
| self.get_logger().fatal("'model_file' parameter must be set for dqn_test.") | |
| sys.exit(1) | |
| use_gpu = self.get_parameter('use_gpu').get_parameter_value().bool_value |
| with open(json_path) as outfile: | ||
| param = json.load(outfile) | ||
| self.epsilon = param.get('epsilon', self.epsilon) | ||
| self.step_counter = param.get('step_counter', self.step_counter) | ||
| self.load_episode = param.get('trained_episodes', self.load_episode) |
There was a problem hiding this comment.
The code for loading parameters from the JSON file does not handle potential json.JSONDecodeError if the file is corrupted. This could cause the node to crash. It's good practice to wrap the json.load() call in a try...except block to handle this case gracefully.
| with open(json_path) as outfile: | |
| param = json.load(outfile) | |
| self.epsilon = param.get('epsilon', self.epsilon) | |
| self.step_counter = param.get('step_counter', self.step_counter) | |
| self.load_episode = param.get('trained_episodes', self.load_episode) | |
| try: | |
| with open(json_path) as outfile: | |
| param = json.load(outfile) | |
| self.epsilon = param.get('epsilon', self.epsilon) | |
| self.step_counter = param.get('step_counter', self.step_counter) | |
| self.load_episode = param.get('trained_episodes', self.load_episode) | |
| except json.JSONDecodeError: | |
| self.get_logger().error(f'Error decoding JSON from {json_path}. Using default values.') |
| idx = 1 | ||
| while True: | ||
| model_path = os.path.join( | ||
| self.model_dir_path, | ||
| f'model{idx}.h5' | ||
| ) | ||
| json_path = os.path.join( | ||
| self.model_dir_path, | ||
| 'stage' + str(self.stage) + '_episode' + str(episode) + '.json' | ||
| ), | ||
| 'w' | ||
| ) as outfile: | ||
| f'model{idx}.json' | ||
| ) | ||
| if not os.path.exists(model_path): | ||
| break | ||
| idx += 1 | ||
| self.model.save(model_path) | ||
| with open(json_path, 'w') as outfile: | ||
| json.dump(param_dictionary, outfile) |
There was a problem hiding this comment.
The current model saving logic creates a new model file (model1.h5, model2.h5, etc.) every 100 episodes without deleting old ones. This can lead to a large number of files and consume significant disk space during long training sessions. If this is not the intended behavior, you might consider a strategy to limit the number of saved checkpoints, such as checkpoint rotation or naming files with the episode number to allow for overwriting.
There was a problem hiding this comment.
Pull request overview
This PR bumps the package version from 1.0.1 to 1.0.2 and introduces dependency management improvements through lazy TensorFlow imports, preparing the package for ROS distro release. The key changes include transitioning from command-line arguments to ROS parameters, adding GPU configuration options, and improving model loading/saving functionality.
Key Changes:
- Implemented lazy imports for TensorFlow modules to resolve dependency issues
- Migrated from system arguments (sys.argv) to ROS parameters for configuration
- Added use_gpu and model_file parameters for better control over training and testing
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| turtlebot3_machine_learning/package.xml | Version bump to 1.0.2 and added new author |
| turtlebot3_machine_learning/CHANGELOG.rst | Added changelog entry for 1.0.2 release |
| turtlebot3_dqn/turtlebot3_dqn/dqn_test.py | Implemented lazy TensorFlow imports and migrated to ROS parameters |
| turtlebot3_dqn/turtlebot3_dqn/dqn_agent.py | Implemented lazy imports, ROS parameters, and improved model saving with sequential naming |
| turtlebot3_dqn/setup.py | Version bump, new author, and explicit dependency specifications |
| turtlebot3_dqn/saved_model/stage1_episode600.json | Removed old model JSON file |
| turtlebot3_dqn/saved_model/model1.json | Added new model JSON with updated parameter names |
| turtlebot3_dqn/package.xml | Version bump, new author, and removed explicit Python dependency declarations |
| turtlebot3_dqn/CHANGELOG.rst | Added changelog entry for 1.0.2 release |
| README.md | Updated branch information and added coin_d4_driver to project list |
| .github/workflows/ros-ci.yml | Added rosdep check step to verify dependency resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.get_logger().error('Loaded model episode exceeds max training episodes.') | ||
| raise ValueError('Loaded model episode exceeds max training episodes.') |
There was a problem hiding this comment.
The error message states "Loaded model episode exceeds max training episodes" but the condition checks if load_episode >= max_training_episodes. This means the error will trigger even when they are equal, not just when it exceeds. Either the message should be updated to "Loaded model episode meets or exceeds max training episodes" or the condition should be changed to load_episode > max_training_episodes.
| self.get_logger().error('Loaded model episode exceeds max training episodes.') | |
| raise ValueError('Loaded model episode exceeds max training episodes.') | |
| self.get_logger().error('Loaded model episode meets or exceeds max training episodes.') | |
| raise ValueError('Loaded model episode meets or exceeds max training episodes.') |
| def get_action(self, state): | ||
| state = numpy.asarray(state) | ||
| q_values = self.model.predict(state.reshape(1, -1), verbose=0) | ||
| q_values = self.model.predict(state.reshape(1, -1), verbose=self.verbose) |
There was a problem hiding this comment.
The 'verbose' parameter in model.predict() expects an integer (0, 1, or 2) to control logging verbosity, but self.verbose is a boolean. When verbose=True, it will be interpreted as 1 (showing progress bar), and when verbose=False, it will be 0 (silent). While this works, it would be clearer to explicitly convert the boolean to the expected integer values (e.g., 1 if self.verbose else 0) or declare the parameter as an integer type to match the API expectations.
| q_values = self.model.predict(state.reshape(1, -1), verbose=self.verbose) | |
| q_values = self.model.predict(state.reshape(1, -1), | |
| verbose=1 if self.verbose else 0) |
| local_step += 1 | ||
|
|
||
| q_values = self.model.predict(state) | ||
| q_values = self.model.predict(state, verbose=self.verbose) |
There was a problem hiding this comment.
The 'verbose' parameter in model.predict() expects an integer (0, 1, or 2) to control logging verbosity, but self.verbose is a boolean. When verbose=True, it will be interpreted as 1 (showing progress bar), and when verbose=False, it will be 0 (silent). While this works, it would be clearer to explicitly convert the boolean to the expected integer values (e.g., 1 if self.verbose else 0) or declare the parameter as an integer type to match the API expectations.
| q_values = self.model.predict(state, verbose=self.verbose) | |
| verbose_level = 1 if self.verbose else 0 | |
| q_values = self.model.predict(state, verbose=verbose_level) |
| <author email="kkjong@robotis.com">Gilbert</author> | ||
| <author email="dddoggi1207@gmail.com">ChanHyeong Lee</author> | ||
| <author email="kimhg@robotis.com">Hyungyu Kim</author> | ||
| <exec_depend>python3-pip</exec_depend> |
There was a problem hiding this comment.
The dependencies 'python-tensorflow-pip' and 'python3-numpy' have been removed from exec_depend, but these are now specified in setup.py's install_requires. While this may work, it's important to ensure that the ROS dependency resolution system can properly handle Python package dependencies specified only in setup.py. Consider whether these should remain as exec_depend entries for better ROS ecosystem integration.
| <exec_depend>python3-pip</exec_depend> | |
| <exec_depend>python3-pip</exec_depend> | |
| <exec_depend>python-tensorflow-pip</exec_depend> | |
| <exec_depend>python3-numpy</exec_depend> |
| 'tensorflow==2.19.0', | ||
| 'numpy==1.26.4', | ||
| 'scipy==1.10.1', | ||
| 'keras==3.9.2', |
There was a problem hiding this comment.
Pinning specific versions of TensorFlow, NumPy, SciPy, and Keras in install_requires can cause dependency conflicts and make the package less flexible. These exact versions may not be compatible with all systems or other dependencies. Consider using version ranges (e.g., 'tensorflow>=2.19.0,<3.0.0') or removing the version constraints if the code is compatible with multiple versions.
| 'tensorflow==2.19.0', | |
| 'numpy==1.26.4', | |
| 'scipy==1.10.1', | |
| 'keras==3.9.2', | |
| 'tensorflow>=2.19.0,<3.0.0', | |
| 'numpy>=1.26.4,<2.0.0', | |
| 'scipy>=1.10.1,<2.0.0', | |
| 'keras>=3.9.2,<4.0.0', |
| idx = 1 | ||
| while True: | ||
| model_path = os.path.join( | ||
| self.model_dir_path, | ||
| f'model{idx}.h5' | ||
| ) | ||
| json_path = os.path.join( | ||
| self.model_dir_path, | ||
| 'stage' + str(self.stage) + '_episode' + str(episode) + '.json' | ||
| ), | ||
| 'w' | ||
| ) as outfile: | ||
| f'model{idx}.json' | ||
| ) | ||
| if not os.path.exists(model_path): | ||
| break | ||
| idx += 1 | ||
| self.model.save(model_path) | ||
| with open(json_path, 'w') as outfile: | ||
| json.dump(param_dictionary, outfile) |
There was a problem hiding this comment.
The model saving logic increments the model file index without bound. This could lead to an accumulation of model files over time. Consider implementing a cleanup strategy or limiting the number of saved models (e.g., keeping only the last N models) to prevent disk space issues during long training runs.
| tensorflow.config.set_visible_devices([], 'GPU') | ||
|
|
||
| LOGGING = True | ||
| current_time = datetime.datetime.now().strftime('[%mm%dd-%H:%M]') |
There was a problem hiding this comment.
The datetime format string '%mm%dd' contains lowercase 'm' which represents minutes, not months. This should be '%m%d' to properly format the date as month and day. The current format will produce incorrect timestamps like '[01m06d-12:34]' instead of the intended '[0106-12:34]'.
| current_time = datetime.datetime.now().strftime('[%mm%dd-%H:%M]') | |
| current_time = datetime.datetime.now().strftime('[%m%d-%H:%M]') |
Dependency issues have been resolved through lazy imports, and the package is now ready for ROS distro release.
Additionally, updates have been made to address multiple PRs.