feat: run params query#11
Conversation
- Introduced a new `params` target for the `query` command to allow users to filter and retrieve run parameters from Aim repositories. - Updated command help and README to reflect the new functionality and usage examples. - Enhanced query parsing to handle `--param` options for selecting specific parameters. - Added comprehensive tests to ensure the correctness of the new parameter querying feature and its integration with existing functionalities. - Updated `.gitignore` to include additional environment and temporary files.
![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 query params feature to aimx, enabling users to compare run-level parameters through terminal tables, plain text, or JSON. The implementation includes new bridge logic for metadata extraction and flattening, along with updated CLI parsing and documentation. Review feedback suggests enhancing the robustness of these changes by ensuring safe sorting of mixed-type metadata keys, making value truncation optional to better support scriptable workflows, and sanitizing output in the plain-text renderer to prevent newlines or tabs from breaking the tab-separated format.
| def flatten_params(params: dict[str, Any], *, prefix: str = "") -> dict[str, Any]: | ||
| """Flatten nested dictionaries into stable dotted parameter keys.""" | ||
| flattened: dict[str, Any] = {} | ||
| for key in sorted(params): |
There was a problem hiding this comment.
Using sorted(params) without a key can lead to a TypeError in Python 3 if the dictionary contains keys of mixed types (e.g., int and str). While Aim metadata keys are typically strings, Aim allows arbitrary JSON-like data in run attributes. Using key=str ensures the sorting is stable and safe across mixed types.
| for key in sorted(params): | |
| for key in sorted(params, key=str): |
| def _display(value: Any, *, max_len: int = 48) -> str: | ||
| if value is None: | ||
| text = "null" | ||
| elif isinstance(value, bool): | ||
| text = "true" if value else "false" | ||
| else: | ||
| text = str(value) | ||
| if len(text) > max_len: | ||
| return f"{text[: max_len - 1]}…" | ||
| return text |
There was a problem hiding this comment.
The _display function currently truncates values to 48 characters by default. While this is appropriate for human-readable table output, it is problematic for oneline mode, which is intended for scriptability and shell pipelines where the full value is often required. Making max_len optional allows renderers to disable truncation when necessary.
| def _display(value: Any, *, max_len: int = 48) -> str: | |
| if value is None: | |
| text = "null" | |
| elif isinstance(value, bool): | |
| text = "true" if value else "false" | |
| else: | |
| text = str(value) | |
| if len(text) > max_len: | |
| return f"{text[: max_len - 1]}…" | |
| return text | |
| def _display(value: Any, *, max_len: int | None = 48) -> str: | |
| if value is None: | |
| text = "null" | |
| elif isinstance(value, bool): | |
| text = "true" if value else "false" | |
| else: | |
| text = str(value) | |
| if max_len is not None and len(text) > max_len: | |
| return f"{text[: max_len - 1]}…" | |
| return text |
| def render_params_oneline(rows: list[RunParams], header_info: dict[str, Any]) -> str: | ||
| repo = header_info.get("repo", "") | ||
| keys, _ = _keys_for_display(rows, limit=None) | ||
| lines: list[str] = [] | ||
| for row in rows: | ||
| cells = [ | ||
| repo, | ||
| _short_hash(row.run.hash), | ||
| row.run.experiment or "", | ||
| row.run.name or "", | ||
| ] | ||
| for key in keys: | ||
| value = _display(row.params[key]) if key in row.params else "-" | ||
| cells.append(f"{key}={value}") | ||
| if not keys: | ||
| cells.append("params=-") | ||
| lines.append("\t".join(cells)) | ||
| return "\n".join(lines) |
There was a problem hiding this comment.
The oneline renderer should ensure that the output remains a single tab-separated line per run to maintain compatibility with shell tools like awk or cut. Currently, it does not handle values containing newlines or tabs, which would break the format. Additionally, it should disable the default truncation in _display to preserve full parameter values for scripting.
| def render_params_oneline(rows: list[RunParams], header_info: dict[str, Any]) -> str: | |
| repo = header_info.get("repo", "") | |
| keys, _ = _keys_for_display(rows, limit=None) | |
| lines: list[str] = [] | |
| for row in rows: | |
| cells = [ | |
| repo, | |
| _short_hash(row.run.hash), | |
| row.run.experiment or "", | |
| row.run.name or "", | |
| ] | |
| for key in keys: | |
| value = _display(row.params[key]) if key in row.params else "-" | |
| cells.append(f"{key}={value}") | |
| if not keys: | |
| cells.append("params=-") | |
| lines.append("\t".join(cells)) | |
| return "\n".join(lines) | |
| def render_params_oneline(rows: list[RunParams], header_info: dict[str, Any]) -> str: | |
| repo = header_info.get("repo", "") | |
| keys, _ = _keys_for_display(rows, limit=None) | |
| lines: list[str] = [] | |
| for row in rows: | |
| cells = [ | |
| repo.replace("\t", " ").replace("\n", " "), | |
| _short_hash(row.run.hash), | |
| (row.run.experiment or "").replace("\t", " ").replace("\n", " "), | |
| (row.run.name or "").replace("\t", " ").replace("\n", " "), | |
| ] | |
| for key in keys: | |
| raw_val = _display(row.params[key], max_len=None) if key in row.params else "-" | |
| val = raw_val.replace("\t", " ").replace("\n", " ") | |
| cells.append(f"{key}={val}") | |
| if not keys: | |
| cells.append("params=-") | |
| lines.append("\t".join(cells)) | |
| return "\n".join(lines) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b3c778de3
ℹ️ 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".
| row.run.name or "", | ||
| ] | ||
| for key in keys: | ||
| value = _display(row.params[key]) if key in row.params else "-" |
There was a problem hiding this comment.
aimx query params --plain/--oneline is documented as shell-friendly output, but each parameter value is currently passed through _display, which truncates anything longer than 48 characters with an ellipsis. This silently loses information for long strings/lists and makes downstream comparisons or exports incorrect, because users cannot recover the original value from plain output.
Useful? React with 👍 / 👎.
| return {str(key): _jsonable(item) for key, item in value.items()} | ||
| if isinstance(value, (list, tuple)): | ||
| return [_jsonable(item) for item in value] | ||
| if value is None or isinstance(value, (str, int, float, bool)): |
There was a problem hiding this comment.
The params JSON path passes Python floats through unchanged, so a run parameter containing NaN/Infinity will be serialized as non-standard JSON tokens by json.dumps (default allow_nan=True). That output is rejected by strict JSON parsers and breaks automation workflows that consume aimx query params --json outside Python.
Useful? React with 👍 / 👎.
1 participant
No description provided.