feat: enhance query functionality with new filtering options and image rendering capabilities#8
Conversation
…e rendering capabilities - Added support for `--epochs` flag in query commands to filter results based on epoch ranges. - Introduced `--head`, `--tail`, and `--every` flags to limit the number of results returned for both metrics and images. - Updated README to reflect new command usage and examples. - Improved image rendering in terminal with inline previews and ensured no graphics bytes are output in non-TTY contexts. - Enhanced tests to cover new features and ensure stability across query functionalities.
![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 implements inline terminal image rendering for aimx query images using the textual-image library and introduces new query capabilities including epoch-based filtering and density subsampling (head, tail, every). The changes encompass a new rendering module, lazy image loading from the Aim SDK, and updated CLI argument handling. Review feedback focuses on ensuring compliance with the functional requirement to display the summary table alongside images, improving the robustness of chronological sorting by utilizing direct object attributes, and streamlining the internal rendering API by removing unused parameters.
| # TTY inline-image extension (US1/US3): only when stdout is a TTY and | ||
| # the rendering path is "rich" (not --json / --plain). | ||
| from aimx.rendering.image_render import detect_capability, plan_render, render_inline # noqa: PLC0415 | ||
|
|
||
| capability = detect_capability() | ||
| if capability.protocol != "disabled": | ||
| plan = plan_render(image_rows, capability, max_images=invocation.max_images) | ||
| inline = render_inline(plan) | ||
| if inline: | ||
| repo = header_info.get("repo", "") | ||
| expr = header_info.get("expression", "") | ||
| total = len(image_rows) | ||
| shown = len(plan.rendered_rows) | ||
| compact_header = ( | ||
| f"Repo: {repo} · {total} matches · images where {expr}" | ||
| ) | ||
| if shown < total: | ||
| compact_header += f" · previewing {shown}" | ||
| compact_header += "\n" | ||
|
|
||
| return QueryCommandResult(exit_status=0, output=compact_header + inline) | ||
|
|
||
| summary = render_image_rich_table(image_rows, header_info, no_color=no_color) | ||
| return QueryCommandResult(exit_status=0, output=summary) |
There was a problem hiding this comment.
The current implementation violates functional requirement FR-005 by omitting the summary metadata table when inline images are rendered. The spec explicitly states that the rich summary table MUST be output first, followed by the image blocks. Additionally, the compact_header introduced here is redundant as render_image_rich_table already provides a similar summary header.
summary = render_image_rich_table(image_rows, header_info, no_color=no_color)
# TTY inline-image extension (US1/US3): only when stdout is a TTY and
# the rendering path is "rich" (not --json / --plain).
from aimx.rendering.image_render import detect_capability, plan_render, render_inline # noqa: PLC0415
capability = detect_capability()
if capability.protocol != "disabled":
plan = plan_render(image_rows, capability, max_images=invocation.max_images)
inline = render_inline(plan)
if inline:
return QueryCommandResult(exit_status=0, output=summary + "\n" + inline)
return QueryCommandResult(exit_status=0, output=summary)| epoch_value = context.get("epoch") | ||
| if epoch_value is None: | ||
| epoch_value = _first_iter_value(getattr(image, "epochs", None)) | ||
| step_value = _call_or_value(getattr(image, "first_step", None)) |
There was a problem hiding this comment.
The extraction of epoch and step values for sorting is incomplete. Individual Image objects in Aim 3.x have epoch and step as direct attributes. These should be prioritized over context lookups or sequence-level accessors (like first_step) to ensure that images are correctly sorted chronologically within a run.
| epoch_value = context.get("epoch") | |
| if epoch_value is None: | |
| epoch_value = _first_iter_value(getattr(image, "epochs", None)) | |
| step_value = _call_or_value(getattr(image, "first_step", None)) | |
| epoch_value = getattr(image, "epoch", None) | |
| if epoch_value is None: | |
| epoch_value = context.get("epoch") | |
| if epoch_value is None: | |
| epoch_value = _first_iter_value(getattr(image, "epochs", None)) | |
| step_value = getattr(image, "step", None) | |
| if step_value is None: | |
| step_value = _call_or_value(getattr(image, "first_step", None)) |
| def _render_pil_image( | ||
| pil_img: "PILImage.Image", | ||
| _target_width: int, | ||
| _max_height: int, | ||
| ) -> "PILImage.Image": |
There was a problem hiding this comment.
The parameters _target_width and _max_height are unused in the function body. Since resizing is delegated to textual-image (as noted in the docstring), these parameters should be removed from the signature to simplify the internal API.
def _render_pil_image(
pil_img: "PILImage.Image",
) -> "PILImage.Image":|
|
||
| try: | ||
| pil_img = accessor() | ||
| pil_img = _render_pil_image(pil_img, plan.target_width, plan.max_height) |
There was a problem hiding this comment.
Update the call site to match the simplified signature of _render_pil_image (removing unused width/height arguments).
| pil_img = _render_pil_image(pil_img, plan.target_width, plan.max_height) | |
| pil_img = _render_pil_image(pil_img) |
![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: 31a4d89955
ℹ️ 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".
| if shown < total: | ||
| compact_header += f" · previewing {shown}" | ||
| compact_header += "\n" | ||
|
|
||
| return QueryCommandResult(exit_status=0, output=compact_header + inline) |
There was a problem hiding this comment.
When inline rendering succeeds, this branch returns compact_header + inline immediately instead of including render_image_rich_table(...), so interactive TTY users no longer see the full metadata table. With the default --max-images=6, any matches beyond the preview cap become invisible except for a count, which is a functional regression from previous output and prevents inspecting non-previewed rows.
Useful? React with 👍 / 👎.
| console = Console( | ||
| file=buf, | ||
| force_terminal=True, | ||
| width=plan.capability.columns, | ||
| highlight=False, |
There was a problem hiding this comment.
Inline rendering always constructs a Console with force_terminal=True and no no_color override, so metadata headers are still ANSI-styled even when users pass --no-color. In TTY runs this breaks the expected contract that no-color disables styled text output, and it also risks leaking escape sequences when users capture interactive output.
Useful? React with 👍 / 👎.
- Updated version number to 0.3.1 in project files. - Improved image query command to preserve full rich table while displaying inline previews. - Added a new test to ensure that interactive inline previews do not hide rows beyond the preview cap.
1 participant
--epochsflag in query commands to filter results based on epoch ranges.--head,--tail, and--everyflags to limit the number of results returned for both metrics and images.