Normalize hardware response keys across APIs #2
Conversation
Summary of ChangesHello @jithinAB, 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 aims to enhance the robustness and predictability of hardware data across the API by implementing a consistent key normalization strategy. By ensuring all hardware dictionaries returned by both FastAPI and Flask endpoints have standardized lowercase keys, it simplifies data consumption for clients and prevents potential issues arising from varied key casing in source data. Highlights
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 a utility function to normalize keys in hardware data dictionaries, ensuring consistency across API responses from both FastAPI and Flask routes. My review identifies a potential issue in the new normalization logic and an opportunity for refactoring.
Specifically, I've suggested an improvement to normalize_hardware_keys to handle mixed-case keys in addition to uppercase ones, which is crucial for ensuring all data is correctly processed. I've also recommended refactoring duplicated code in the FastAPI routes into a helper function to improve maintainability.
| def normalize_hardware_keys(hardware: Dict[str, Any]) -> Dict[str, Any]: | ||
| """Ensure lowercase hardware keys exist for their uppercase counterparts. | ||
| Some hardware records may come from sources that use uppercase keys (e.g., | ||
| ``FLOPS`` instead of ``flops``). This helper preserves the original keys | ||
| while populating the expected lowercase versions when they are missing so | ||
| that API serializers can rely on a consistent shape. | ||
| """ | ||
| if hardware is None: | ||
| return {} | ||
|
|
||
| normalized = dict(hardware) | ||
| for key, value in hardware.items(): | ||
| if isinstance(key, str) and key.isupper(): | ||
| lower_key = key.lower() | ||
| if lower_key not in normalized: | ||
| normalized[lower_key] = value | ||
| return normalized |
There was a problem hiding this comment.
The current implementation of normalize_hardware_keys only handles fully uppercase keys (e.g., FLOPS) because of key.isupper(). However, other parts of the codebase suggest that keys can also be in PascalCase or mixed case (e.g., Memory_size, Flops). These keys would not be correctly normalized.
To make this utility more robust and handle all non-lowercase keys, I suggest modifying the logic to check if key.lower() != key. This will correctly handle uppercase, PascalCase, and mixed-case keys. I've also updated the docstring to reflect this broader scope.
This change will ensure that all expected keys are normalized, making the API responses more consistent.
| def normalize_hardware_keys(hardware: Dict[str, Any]) -> Dict[str, Any]: | |
| """Ensure lowercase hardware keys exist for their uppercase counterparts. | |
| Some hardware records may come from sources that use uppercase keys (e.g., | |
| ``FLOPS`` instead of ``flops``). This helper preserves the original keys | |
| while populating the expected lowercase versions when they are missing so | |
| that API serializers can rely on a consistent shape. | |
| """ | |
| if hardware is None: | |
| return {} | |
| normalized = dict(hardware) | |
| for key, value in hardware.items(): | |
| if isinstance(key, str) and key.isupper(): | |
| lower_key = key.lower() | |
| if lower_key not in normalized: | |
| normalized[lower_key] = value | |
| return normalized | |
| def normalize_hardware_keys(hardware: Dict[str, Any]) -> Dict[str, Any]: | |
| """Ensure lowercase hardware keys exist for their non-lowercase counterparts. | |
| Some hardware records may come from sources that use uppercase or mixed-case | |
| keys (e.g., ``FLOPS`` or ``Memory_size`` instead of ``flops`` or ``memory_size``). | |
| This helper preserves the original keys while populating the expected lowercase | |
| versions when they are missing so that API serializers can rely on a consistent shape. | |
| """ | |
| if hardware is None: | |
| return {} | |
| normalized = dict(hardware) | |
| for key, value in hardware.items(): | |
| if isinstance(key, str): | |
| lower_key = key.lower() | |
| if key != lower_key and lower_key not in normalized: | |
| normalized[lower_key] = value | |
| return normalized |
| enhanced_results = [] | ||
| for hw in results: | ||
| normalized_hw = normalize_hardware_keys(hw) | ||
|
|
||
| # Calculate price indicator if specs are available | ||
| flops = hw.get('flops', 0) | ||
| memory_size = hw.get('memory_size', 0) | ||
| memory_bw = hw.get('memory_bw', 0) | ||
| flops = normalized_hw.get('flops', 0) | ||
| memory_size = normalized_hw.get('memory_size', 0) | ||
| memory_bw = normalized_hw.get('memory_bw', 0) | ||
|
|
||
| if flops > 0 and memory_size > 0 and memory_bw > 0: | ||
| hw['price_approx'] = BudHardware.calculate_price_indicator( | ||
| normalized_hw['price_approx'] = BudHardware.calculate_price_indicator( | ||
| flops=flops, | ||
| memory_gb=memory_size, | ||
| bandwidth_gbs=memory_bw | ||
| ) | ||
| else: | ||
| hw['price_approx'] = None | ||
| enhanced_results.append(hw) | ||
| normalized_hw['price_approx'] = None | ||
|
|
||
| enhanced_results.append(normalized_hw) |
There was a problem hiding this comment.
There is significant code duplication between this loop in list_hardware and a similar loop in filter_hardware (lines 282-300). To improve maintainability and reduce redundancy, consider extracting this logic into a private helper function.
For example, you could create a function _enhance_hardware_results:
def _enhance_hardware_results(hardware_list: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
enhanced_results = []
for hw in hardware_list:
normalized_hw = normalize_hardware_keys(hw)
# Calculate price indicator if specs are available
flops = normalized_hw.get('flops', 0)
memory_size = normalized_hw.get('memory_size', 0)
memory_bw = normalized_hw.get('memory_bw', 0)
if flops > 0 and memory_size > 0 and memory_bw > 0:
normalized_hw['price_approx'] = BudHardware.calculate_price_indicator(
flops=flops,
memory_gb=memory_size,
bandwidth_gbs=memory_bw
)
else:
normalized_hw['price_approx'] = None
enhanced_results.append(normalized_hw)
return enhanced_resultsThen, both list_hardware and filter_hardware can be simplified to:
enhanced_results = _enhance_hardware_results(results)
return [HardwareResponse(**hw) for hw in enhanced_results]
Summary
Testing
https://chatgpt.com/codex/tasks/task_b_68daf0d798348326822297517c697e2a