feat: Refactor TestCaseController algorithm parsing and enhance error handling #320
Conversation
… handling Signed-off-by: vinayak sharma <vinayaks0111@gmail.com>
kubeedge-bot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yoursanonymous The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @yoursanonymous, 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 refactors the algorithm configuration parsing within 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. Changelog
Activity
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
|
kubeedge-bot
added
the
size/M
![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 significantly refactors the _parse_algorithms_config method in TestCaseController, leading to improved error handling, better readability, and reduced redundancy. The changes introduce more specific exception types for clearer diagnostics, separate error handling for YAML parsing and algorithm initialization, and consolidate the algorithm expansion logic into a single, more efficient loop. These improvements enhance the maintainability and user experience by providing more actionable feedback for configuration issues.
| try: | ||
| raw_config = utils.yaml2dict(config_file) | ||
| except Exception as err: | ||
| raise ValueError( | ||
| f"Failed to parse algorithm config file({config_file}): {err}" | ||
| ) from err |
There was a problem hiding this comment.
Separating the YAML parsing into its own try-except block with a ValueError provides much clearer diagnostics. Users will immediately know if their YAML file is malformed, rather than guessing if it's a file issue or an algorithm initialization problem. This significantly improves the debugging experience.
| try: | ||
| config = utils.yaml2dict(config_file) | ||
| algorithm = Algorithm(name, config) | ||
| algorithms.append(algorithm) | ||
| algorithm = Algorithm(name, raw_config) | ||
| except Exception as err: | ||
| raise RuntimeError(f"algorithm config file({config_file} is not supported, " | ||
| f"error: {err}") from err | ||
| raise RuntimeError( | ||
| f"Failed to initialize Algorithm '{name}' from {config_file}: {err}" | ||
| ) from err |
There was a problem hiding this comment.
This dedicated try-except block for Algorithm initialization, distinct from YAML parsing, further improves error clarity. It helps pinpoint whether the issue is with the configuration content itself or how the Algorithm class processes that content, making it easier to identify the root cause of failures.
| if not config_file: | ||
| raise ValueError(f"Algorithm 'url' is missing for algorithm: {name}") |
There was a problem hiding this comment.
This explicit check for a missing 'url' is a great improvement. It provides immediate and clear feedback to the user, preventing later, more obscure errors that might arise from attempting to use a None value as a file path.
| if not utils.is_local_file(config_file): | ||
| raise RuntimeError(f"not found algorithm config file({config_file}) in local") | ||
| raise FileNotFoundError(f"Algorithm config file not found: {config_file}") |
There was a problem hiding this comment.
Using FileNotFoundError instead of a generic RuntimeError is more semantically correct and allows for more specific error handling by callers. This makes the error message more precise and helpful.
| # If no modules_list (e.g. no permutations), add the base algorithm | ||
| if not algorithm.modules_list: | ||
| all_expanded_algorithms.append(algorithm) | ||
| continue | ||
|
|
||
| new_algorithms = [] | ||
| for algorithm in algorithms: | ||
| for modules in algorithm.modules_list: | ||
| new_algorithm = copy.deepcopy(algorithm) | ||
| new_algorithm.modules = modules | ||
| new_algorithms.append(new_algorithm) | ||
| all_expanded_algorithms.append(new_algorithm) |
There was a problem hiding this comment.
Consolidating the algorithm expansion logic into a single loop, and directly appending the base algorithm when modules_list is empty, streamlines the code. This reduces redundancy and improves readability compared to the previous two-loop approach, making the logic easier to follow and maintain.
Signed-off-by: vinayak sharma <vinayaks0111@gmail.com>
2 participants
What type of PR is this?
This PR optimizes the algorithm configuration parsing in TestCaseController by consolidating redundant loops and introducing specific error types. This ensures that users receive clear, actionable feedback when their benchmarking configurations are incorrect.
/kind feature
What this PR does / why we need it:
The current implementation of algorithm parsing is difficult to debug and maintain:
Which issue(s) this PR fixes:
Fixes #319