[autorevert] Add hud-like HTML grid rendering for the logged state and autorevert-checker subcommand
#7186
Conversation
…d `autorevert-checker` subcommand
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
bot
added
the
CLA Signed
| @@ -113,6 +113,15 @@ def get_opts() -> argparse.Namespace: | |||
| "Revert mode: skip, log (no side effects), run-notify (side effect), or run-revert (side effect)." | |||
| ), | |||
| ) | |||
| workflow_parser.add_argument( | |||
There was a problem hiding this comment.
this functionality should not be a flag on revert mode, instead a separate entry point that reads from the database and creates this html.
this is because I don't want to couple the autorevert logic to the html generation logic.
This is not safe for a service, it makes more sense for those to be separated entities/actions.
There was a problem hiding this comment.
wdym?
there IS a separate endpoint: hud subcommand that reads and prints from the database
the flag to the autorevert just reuses the functionality to optionally dump the results of the run as html, it's a convenience
| @@ -17,15 +18,16 @@ def autorevert_v2( | |||
| repo_full_name: str = "pytorch/pytorch", | |||
| restart_action: RestartAction = RestartAction.RUN, | |||
| revert_action: RevertAction = RevertAction.LOG, | |||
| ) -> Tuple[List[Signal], List[Tuple[Signal, SignalProcOutcome]]]: | |||
| out_hud: Optional[str] = None, | |||
There was a problem hiding this comment.
if this entry point is going to be used in production, I believe that we should not add the html logic to it. Unnecessary complexity that adds risk in production.
|
|
||
| from ..hud_renderer import build_grid_model, render_html | ||
| from ..signal_extraction import SignalExtractor | ||
| from ..hud_renderer import render_html_from_state |
There was a problem hiding this comment.
hud_renderer generates html by concatenation,
I believe we should move away from this, this is NOT maintainable or easy to understand. Please replace with a proper template compiler engine. It is OK to have the template as a string in a file.
|
|
||
| Returns the output filepath. | ||
| """ | ||
| def _ensure_state_dict(state: RunStatePayload) -> Mapping[str, Any]: |
There was a problem hiding this comment.
please add a strict jsonschema validation here
otherwise errors will be random, criptic and hard to understand if someone provides accidentally the wrong json file.
my recommendation:
Use the same type to typehint, jsonschema validate on load AND on write to the database. In order to avoid bugs poisoning the database.
|
|
||
| Returns the output filepath. | ||
| """ | ||
| def _ensure_state_dict(state: RunStatePayload) -> Mapping[str, Any]: |
There was a problem hiding this comment.
return proper typing, specifying the correct full json structure. Probably it is too complex to put here, but please build typeclasses and typedefinitions.
if you used Any as a type, you're doing it wrong.
| Extracts signals for the given workflows, optionally truncates commit history | ||
| to ignore commits newer than a specific SHA, builds a HUD model, and writes | ||
| an HTML report to `out`. | ||
| RunStatePayload = Union[str, Mapping[str, Any]] |
There was a problem hiding this comment.
same for datatype here, avoid Any, be strict and build the full json datatype.
| """Render the given run-state JSON (string or mapping) to HUD HTML.""" | ||
| state_dict = _ensure_state_dict(state) | ||
| meta = state_dict.get("meta", {}) | ||
| workflows = meta.get("workflows") or [] |
There was a problem hiding this comment.
workflows = meta.get("workflows", [])
| row = rows[0] | ||
| repo = row["repo"] | ||
| workflows = row["workflows"] | ||
| state_json = row["state"] |
features:
pytorch_auto_revertto render run results in the same format:other changes:
Warning
Disclaimer: the code (in particularly the renderer) is mostly vibecoded and is messy, however it's not critical and functions well enough for now
testing
Ran commands locally, including combo of
autorevert-checkerin dry-run mode +hudon its timestamp.