Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor testmo-run-submit-thread #35

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Refactor testmo-run-submit-thread #35

wants to merge 4 commits into from

Conversation

dbarbuzzi
Copy link
Contributor

@dbarbuzzi dbarbuzzi commented Oct 3, 2024 •
edited
Loading

Summary:

These changes refactor the testmo-run-submit-thread action to reduce complexity and improve output (without any functional changes):

  • Total number of variables and amount of nesting has been greatly reduced to simplify reading through the code
  • All inputs are mapped to environment variables and only used that way to help debugging
  • Explicit error messages are displayed for different error scenarios (missing folder, missing files)
    • The action ends immediately if those scenarios are triggered
    • Relevant output (ls output) is confined to those scenarios
    • A summary-level warning is emitted to help bring visibility to this scenario

All that said, a key functional change I want to make is that, if those error scenarios are triggered, to let this step fail instead of silently continue. However, if this is deemed not desired, I will leave things as-is.

Test Plan:

This run has a workflow running three jobs with the current changes: https://github.com/neuralmagic/llm-compressor-testing/actions/runs/11166974501

  • TEST-HAPPY — the happy path, calls the action with a results folder that contains results files. All is well!
  • TEST-MISSING-FOLDER — calls the action with a results folder that doesn't exist. Everything still works as expected including Testmo integration, a warning is present at the summary level, and concise, detailed output is present in the step output.
  • TEST-EMPTY-FOLDER — calls the action with a results folder that exists but does not contain any XML result files. Everything still works as expected including Testmo integration, a warning is present at the summary level, and concise, detailed output is present in the step output.

@derekk-nm
Copy link
Contributor

yeah, these changes could have saved me days of frustration. thank you.
Agreed that it probably makes sense to have this action fail if result dir/files don't exist. I can't think of a present scenario where we should be running this action and there are no results to report.
with that said, it would be good to review code where this action is used to verify that the calling action/workflow will not call this action in the scenario where pytest had an error and didn't generate a results file.

@dbarbuzzi dbarbuzzi marked this pull request as ready for review October 3, 2024 17:51
@dbarbuzzi
Copy link
Contributor Author

with that said, it would be good to review code where this action is used to verify that the calling action/workflow will not call this action in the scenario where pytest had an error and didn't generate a results file.

This was my main concern — in such a case, the pytest step would have either passed or failed at the GHA level, and:

  • If it failed, this action is usually still run because the failure is usually because tests failed. In this case, having this step fail too wouldn't have any significant impact on the run as the run would still fail overall, and then two steps would show failing instead of just one.
  • If it passed, this step would fail and that would be the impactful scenario because the run might have otherwise passed. However, I think this scenario is valid and would bring visibility to a problem with the run configuration. Of course, such a run wouldn't actually be impacted unless it is: (a) not using versioning and just pulling this action from @main, or (b) is updated to a tagged version that includes this change.

Ultimately, though, I didn't include that change (at least, not yet), as the refactor has merit without it, but I think the change would be a good one to include.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dbarbuzzi @derekk-nm