Add --link_existing_gcs_logs for the Results Uploader#42
Add --link_existing_gcs_logs for the Results Uploader#42xianyuanjia wants to merge 1 commit intoandroid:mainfrom
Conversation
Allows the invocation to be linked against Mobly logs already uploaded to a given GCS directory outside of the tool
Summary of ChangesHello @xianyuanjia, 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 enhances the Results Uploader tool by providing a new option to link to existing Mobly logs already present in a Google Cloud Storage (GCS) directory. This avoids redundant uploads when logs are already available, streamlining the process for users who manage their GCS storage independently. The changes involve adding a new command-line flag, refactoring file path retrieval, and updating the Resultstore integration to accommodate this new linking capability. 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. Changelog
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a --link_existing_gcs_logs flag to associate an invocation with pre-uploaded Mobly logs in GCS. The implementation involves adding the new command-line argument, refactoring file path collection, and adjusting the logic in both the main uploader script and the Resultstore client to handle this new case. My review focuses on the correctness of the new logic. I've identified a potential issue with handling multiple Mobly directories when the new flag is used, and a platform-dependency issue in path construction. Overall, the changes are well-structured, but these two points should be addressed to ensure robustness.
| try: | ||
| for idx, mobly_dir in enumerate(mobly_dirs): | ||
| gcs_dir = gcs_base_dir.joinpath(f'dir{idx}') | ||
| gcs_dir = gcs_base_dir.joinpath(f'dir{idx}') if not args.link_existing_gcs_logs else gcs_base_dir |
There was a problem hiding this comment.
This logic could lead to incorrect behavior if args.link_existing_gcs_logs is true and there are multiple Mobly directories found (len(mobly_dirs) > 1). In that case, all targets will point to the same gcs_base_dir, but will be associated with files from different local mobly_dir directories. This is likely not the intended behavior.
To prevent this, I recommend adding a check earlier in the main function to ensure that if link_existing_gcs_logs is used, there is at most one Mobly directory to process. For example, after mobly_dirs is populated:
if args.link_existing_gcs_logs and len(mobly_dirs) > 1:
logging.error(
'--link_existing_gcs_logs can only be used with a single Mobly result directory found within logs_dir.'
)
exit(1)| if args.link_existing_gcs_logs: | ||
| # Use already uploaded Mobly logs in GCS | ||
| logging.info('Using existing Mobly logs in GCS dir: %s', gcs_dir) | ||
| gcs_files += [f'{gcs_dir}/{path}' for path in _get_file_paths_recursively(dir_to_upload)] |
There was a problem hiding this comment.
The path construction f'{gcs_dir}/{path}' is not platform-independent. On Windows, str(gcs_dir) will use backslashes as separators, which will result in mixed-separator GCS paths (e.g., my\gcs/dir/some/file.txt).
To ensure consistent behavior across platforms, you should use .as_posix() to get a POSIX-style path string, similar to how it's used elsewhere in this file.
| gcs_files += [f'{gcs_dir}/{path}' for path in _get_file_paths_recursively(dir_to_upload)] | |
| gcs_files += [f'{gcs_dir.as_posix()}/{path}' for path in _get_file_paths_recursively(dir_to_upload)] |
Allows the invocation to be linked against Mobly logs already uploaded to a given GCS directory outside of the tool