Bump 1.0.2#91
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 significantly enhances the 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 updates the turtlebot3_dqn and turtlebot3_machine_learning packages to version 1.0.2, primarily focusing on improving the Deep Q-Learning (DQN) agent's flexibility and maintainability. Key changes include migrating system arguments to ROS parameters for epsilon_decay, max_training_episodes, model_file, use_gpu, and verbose settings. TensorFlow and Keras modules are now lazily imported to optimize startup performance and handle environments without TensorFlow. The model loading and saving mechanism has been refactored to support loading existing trained models via a model_file parameter, and the saving logic now uses an incremental model{idx}.h5 naming convention, including step_counter and trained_episodes in the associated JSON metadata. The README.md was updated to reflect noetic as a legacy branch and to add coin_d4_driver. Hyungyu Kim was added as a contributor and author across relevant files, and specific versions for tensorflow, numpy, scipy, and keras were added to setup.py. A review comment highlighted that the new model saving logic, which iterates to find the next available index, is not robust and could lead to overwriting models if files are deleted out of order, suggesting a more robust approach using the episode number in the filename for uniqueness.
| 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 logic for naming saved models (model{idx}.h5) is not robust. It iterates to find the first unused index, which can lead to overwriting existing models if files are deleted out of order (e.g., if model1.h5 is deleted but model2.h5 exists, the next save will be model1.h5). A more robust approach would be to use the episode number in the filename, which ensures uniqueness and prevents accidental data loss, making it easier to track and manage models.
| 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) | |
| model_base_name = f'dqn_episode_{episode}' | |
| model_path = os.path.join(self.model_dir_path, f'{model_base_name}.h5') | |
| json_path = os.path.join(self.model_dir_path, f'{model_base_name}.json') | |
| self.model.save(model_path) | |
| with open(json_path, 'w') as outfile: | |
| json.dump(param_dictionary, outfile) |
There was a problem hiding this comment.
Pull request overview
This PR bumps the version to 1.0.2 and introduces significant improvements to dependency management and configuration handling for the TurtleBot3 machine learning package. The changes focus on implementing lazy imports for TensorFlow to resolve dependency issues during package installation and switching from command-line arguments to ROS parameters for better configuration management.
Key changes:
- Implemented lazy imports for TensorFlow and Keras modules to defer dependency loading
- Migrated from system arguments to ROS parameters (model_file, use_gpu, verbose, epsilon_decay, max_training_episodes)
- Added GPU control capability and support for loading pre-trained models
- Updated model saving logic to use incremental naming (model1.h5, model2.h5, etc.)
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 18 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 TensorFlow imports, migrated to ROS parameters, updated model saving/loading logic |
| turtlebot3_dqn/setup.py | Version bump and added explicit dependency versions |
| turtlebot3_dqn/saved_model/stage1_episode600.json | Removed old model JSON file |
| turtlebot3_dqn/saved_model/model1.json | Added new model JSON file with updated parameter names |
| turtlebot3_dqn/package.xml | Version bump, removed rosdep dependencies, added new author |
| turtlebot3_dqn/CHANGELOG.rst | Added changelog entry for 1.0.2 release |
| README.md | Updated active/legacy branch information and added coin_d4_driver link |
| .github/workflows/ros-ci.yml | Added rosdep dependency checking steps to CI workflow |
💡 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 when the loaded model episode exceeds max training episodes could be more helpful. Consider including the actual values in the error message to help users understand what went wrong: f'Loaded model episode ({self.load_episode}) exceeds max training episodes ({self.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( | |
| f'Loaded model episode ({self.load_episode}) exceeds max training episodes ({self.max_training_episodes}).' | |
| ) | |
| raise ValueError( | |
| f'Loaded model episode ({self.load_episode}) exceeds max training episodes ({self.max_training_episodes}).' | |
| ) |
| def _import_tensorflow(): | ||
| try: | ||
| import tensorflow | ||
| from tensorflow.keras.layers import Dense | ||
| from tensorflow.keras.layers import Input | ||
| from tensorflow.keras.losses import MeanSquaredError | ||
| from tensorflow.keras.models import load_model | ||
| from tensorflow.keras.models import Sequential | ||
| from tensorflow.keras.optimizers import Adam | ||
| return ( | ||
| tensorflow, | ||
| Dense, | ||
| Input, | ||
| MeanSquaredError, | ||
| load_model, | ||
| Sequential, | ||
| Adam | ||
| ) |
There was a problem hiding this comment.
The lazy import function lacks documentation explaining why lazy imports are being used and what the function does. Add a docstring explaining that this defers TensorFlow imports to avoid dependency issues during package installation and initial import.
| 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 for model.predict() should be an integer (0, 1, or 2 for different verbosity levels), not a boolean. When self.verbose is False, it will pass 0 which is correct, but when True it will pass 1 (not the boolean True). Consider explicitly converting or setting verbose to 0 or 1 to make the intention clear: verbose=1 if self.verbose else 0.
| 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 for model.predict() should be an integer (0, 1, or 2 for different verbosity levels), not a boolean. When self.verbose is False, it will pass 0 which is correct, but when True it will pass 1 (not the boolean True). Consider explicitly converting or setting verbose to 0 or 1 to make the intention clear: verbose=1 if self.verbose else 0.
| q_values = self.model.predict(state, verbose=self.verbose) | |
| q_values = self.model.predict(state, verbose=1 if self.verbose else 0) |
| current_states = numpy.array([transition[0] for transition in data_in_mini_batch]) | ||
| current_states = current_states.squeeze() | ||
| current_qvalues_list = self.model.predict(current_states) | ||
| current_qvalues_list = self.model.predict(current_states, verbose=self.verbose) |
There was a problem hiding this comment.
The verbose parameter for model.predict() should be an integer (0, 1, or 2 for different verbosity levels), not a boolean. When self.verbose is False, it will pass 0 which is correct, but when True it will pass 1 (not the boolean True). Consider explicitly converting or setting verbose to 0 or 1 to make the intention clear: verbose=1 if self.verbose else 0.
| 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-%H:%M]' contains a typo. The '%mm%dd' should be '%m%d' (single percent signs). This will result in literal 'm' and 'd' characters being printed instead of the month and day values. The correct format should be '[%m%d-%H:%M]'.
| current_time = datetime.datetime.now().strftime('[%mm%dd-%H:%M]') | |
| current_time = datetime.datetime.now().strftime('[%m%d-%H:%M]') |
| 'tensorflow==2.19.0', | ||
| 'numpy==1.26.4', | ||
| 'scipy==1.10.1', | ||
| 'keras==3.9.2', |
There was a problem hiding this comment.
The version specifications for TensorFlow, numpy, scipy, and keras are pinned to exact versions. This may cause compatibility issues as exact version pinning can be overly restrictive. Consider using minimum versions with upper bounds (e.g., 'tensorflow>=2.19.0,<3.0.0') to allow for bug fixes and security patches while maintaining compatibility.
| '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', |
| result = numpy.argmax(self.model.predict(state, verbose=self.verbose)) | ||
| else: | ||
| result = numpy.argmax(self.model.predict(state)) | ||
| result = numpy.argmax(self.model.predict(state, verbose=self.verbose)) |
There was a problem hiding this comment.
The verbose parameter for model.predict() should be an integer (0, 1, or 2 for different verbosity levels), not a boolean. When self.verbose is False, it will pass 0 which is correct, but when True it will pass 1 (not the boolean True). Consider explicitly converting or setting verbose to 0 or 1 to make the intention clear: verbose=1 if self.verbose else 0.
| next_states = numpy.array([transition[3] for transition in data_in_mini_batch]) | ||
| next_states = next_states.squeeze() | ||
| next_qvalues_list = self.target_model.predict(next_states) | ||
| next_qvalues_list = self.target_model.predict(next_states, verbose=self.verbose) |
There was a problem hiding this comment.
The verbose parameter for model.predict() should be an integer (0, 1, or 2 for different verbosity levels), not a boolean. When self.verbose is False, it will pass 0 which is correct, but when True it will pass 1 (not the boolean True). Consider explicitly converting or setting verbose to 0 or 1 to make the intention clear: verbose=1 if self.verbose else 0.
| model_file | ||
| ) | ||
| if self.use_pretrained_model: | ||
| self.model.set_weights(self.load_model(model_path).get_weights()) |
There was a problem hiding this comment.
When use_pretrained_model is True but the model file doesn't exist, the load_model call will raise an exception that isn't caught. Consider adding file existence checking before loading the model, or wrapping the load_model call in a try-except block with a clear error message to guide the user.
| self.model.set_weights(self.load_model(model_path).get_weights()) | |
| if not os.path.exists(model_path): | |
| self.get_logger().error(f'Pretrained model file not found: {model_path}') | |
| raise FileNotFoundError(f'Pretrained model file not found: {model_path}') | |
| try: | |
| pretrained_model = self.load_model(model_path) | |
| except Exception as exc: | |
| self.get_logger().error( | |
| f'Failed to load pretrained model from {model_path}: {exc}' | |
| ) | |
| raise | |
| self.model.set_weights(pretrained_model.get_weights()) |
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.