Add safe hparams passthrough to RunMeta#10
Conversation
![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 the extraction of hyperparameters (hparams) into the RunMeta dataclass, supported by a new helper function _as_plain_dict that converts various object types into plain dictionaries. Unit tests have been added to verify the extraction logic for different input types and error scenarios. Feedback was provided to simplify the _as_plain_dict function by removing redundant type checks and flattening the conversion logic for better readability.
| if value is None: | ||
| return None | ||
| if isinstance(value, dict): | ||
| return dict(value) | ||
|
|
||
| to_dict = getattr(value, "to_dict", None) | ||
| if callable(to_dict): | ||
| try: | ||
| converted = to_dict() | ||
| except Exception: # noqa: BLE001 | ||
| return None | ||
| if isinstance(converted, dict): | ||
| return dict(converted) | ||
| try: | ||
| return dict(converted) | ||
| except Exception: # noqa: BLE001 | ||
| return None | ||
|
|
||
| try: | ||
| return dict(value) | ||
| except Exception: # noqa: BLE001 | ||
| return None |
There was a problem hiding this comment.
The _as_plain_dict function contains redundant logic and can be simplified while maintaining the same behavior. Specifically, the check if isinstance(converted, dict) followed by dict(converted) is redundant because dict() can be safely called on a dictionary. The logic can be flattened to improve readability.
| if value is None: | |
| return None | |
| if isinstance(value, dict): | |
| return dict(value) | |
| to_dict = getattr(value, "to_dict", None) | |
| if callable(to_dict): | |
| try: | |
| converted = to_dict() | |
| except Exception: # noqa: BLE001 | |
| return None | |
| if isinstance(converted, dict): | |
| return dict(converted) | |
| try: | |
| return dict(converted) | |
| except Exception: # noqa: BLE001 | |
| return None | |
| try: | |
| return dict(value) | |
| except Exception: # noqa: BLE001 | |
| return None | |
| if value is None: | |
| return None | |
| # If it's not already a dict, try to use to_dict() if available | |
| if not isinstance(value, dict): | |
| to_dict = getattr(value, "to_dict", None) | |
| if callable(to_dict): | |
| try: | |
| value = to_dict() | |
| except Exception: # noqa: BLE001 | |
| return None | |
| # Final attempt to cast to a plain dict (handles dicts, mappings, or iterables of pairs) | |
| try: | |
| return dict(value) | |
| except Exception: # noqa: BLE001 | |
| return None |
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Breezy!
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Motivation
run.hparamsdo not break queries.Description
RunMetawithparams: dict[str, Any] | Noneand populated it fromrun.hparamsin_extract_run_meta(file:src/aimx/aim_bridge/metric_stats.py)._as_plain_dict()to perform best-effort conversion of mapping-like objects by checking fordict, callingto_dict()when available, then falling back todict(...), and returningNoneon any conversion failure to avoid query errors._extract_run_metabeing used by bothcollect_metric_series()andcollect_image_series()so both paths includeparams.tests/unit/test_metric_stats.pyto cover nativedictpassthrough,to_dict()conversion, and graceful fallback when conversion raises.Testing
uv run pytest tests/unit/test_metric_stats.pyand all tests passed (21 passed).Codex Task