Skip to content

fix: always extract the last line of the sandbox output as the metric#89

Merged
hiverge-robot merged 4 commits intohiverge:mainfrom
kerthcet:fix/parse-output
Jan 20, 2026
Merged

fix: always extract the last line of the sandbox output as the metric#89
hiverge-robot merged 4 commits intohiverge:mainfrom
kerthcet:fix/parse-output

Conversation

@kerthcet
Copy link
Collaborator

No description provided.

Signed-off-by: kerthcet <kerthcet@gmail.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
@hiverge-robot hiverge-robot added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 10, 2026
"Run command failed: %s.", e
)
try:
# If the script leaves checkpointed json data, find and return it
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any code related to the checkpoint in sandbox, we have the mechanism in coordinator, not in sandbox I think.

if process.returncode != 0:
raise FunctionExecutionError(f"Error: {stderr}")
return stdout
return stdout.strip().splitlines()[-1] # Return only the last line of output
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always take the last line of the output as metrics as one solution as we discussed last Friday, WDYT? @brp-hiverge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it always going to be only one line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my experience, we always use the last line of the output. And for json.dumps(), it's always one line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that json.dumps() indeed returns one line, but what if we have a field in the json that contains a multiline content (e.g. feedback)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the newline is escaped as \n, rather than an actual line break. So what you propose makes a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \n in json.dumps() will not lead to a new line.

Signed-off-by: kerthcet <kerthcet@gmail.com>
@kerthcet
Copy link
Collaborator Author

ping @brp-hiverge

This reverts commit c778bcd.
@ky-hiverge
Copy link
Member

PTAL @brp-hiverge

@brp-hiverge
Copy link
Contributor

/lgtm

@hiverge-robot hiverge-robot added the lgtm Looks good to me, indicates that a PR is ready to be merged. label Jan 16, 2026
@ky-hiverge
Copy link
Member

/kind bug

@hiverge-robot hiverge-robot added bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jan 20, 2026
@hiverge-robot hiverge-robot merged commit ab9e08f into hiverge:main Jan 20, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bug Categorizes issue or PR as related to a bug. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants