Add log upload support for build target failed by erman-gurses · Pull Request #587 · ROCm/TheRock

erman-gurses · 2025-05-09T21:41:06Z

This PR adds the log upload support when the build target failed within the workflow. It is not using teatime.py script

erman-gurses · 2025-05-09T22:02:03Z

The question is how would we test this? Maybe, I can purposely break purposely in the build and see how it behaves.

erman-gurses · 2025-05-10T02:48:46Z

Initial test is passed.
https://github.com/ROCm/TheRock/actions/runs/14939987951/job/41975427445

ScottTodd · 2025-05-12T16:39:11Z

Why create an empty directory and then upload it?

What if it does not exist? Isn't it possible?

If there aren't any files to upload, don't upload any files?

Generally, think through the process here and what the state should be at each step:

Job starts. Do we set up some metadata here and create a folder in S3 with that metadata now? Did a prior job already do that and are we just writing into an already established location?

Job installs requirements, initializes caches, etc.

Job starts building. Build logs start to get produced. Build artifacts start to get produced.

Job finishes building. Artifacts and logs are uploaded.

What should happen if the build fails? Should we upload partial artifacts? What about logs?
What should happen if the job is cancelled? Should we upload partial artifacts? What about logs?

Notice that with log (and artifact) streaming, the answers change.

What should happen if the build fails? Should we upload partial artifacts? What about logs?
What should happen if the job is cancelled? Should we upload partial artifacts? What about logs?

So if the build fails in the beginning, there would not be any log to upload - I assume the time even the build/logs does not exist

If the build fails in the middle, there must be some log to upload so build/logs exists and upload whatever it exist for logs.

If the job is canceled, in the middle of the building - we still should upload what we have as log.

I am searching the case 3 if it is possible - but @ScottTodd what is your opinion?

My understanding from my small research, always() does work for the user triggered cancelations If I understand correctly.

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always

Causes the step to always execute, and returns true, even when canceled. The always expression is best used at the step level or on tasks that you expect to run even when a job is canceled. For example, you can use always to send logs even when a job is canceled.

Tested the canceling case - worked fine and uploaded the logs: https://github.com/ROCm/TheRock/actions/runs/14985473348/job/42098511841

erman-gurses · 2025-05-13T06:12:06Z

Tested the normal working case:
https://github.com/ROCm/TheRock/actions/runs/14986147067/job/42100345141

marbre

There are some issues that need to be addressed before this can be merged. You might want to ask @ScottTodd for approval if you need another review today.

marbre

There is a missing terminator thus the workflow probably fails with an bash syntax error. Furthermore, with #608 landed this now also needs to address Windows.

erman-gurses · 2025-05-15T05:35:16Z

@ScottTodd @marbre
The last test completed fine (after I addressed all of the comments) with proper log files:
https://github.com/ROCm/TheRock/actions/runs/15035331614

@marbre, currently, Build Windows Packages on the main branch does not complete successfully.
https://github.com/ROCm/TheRock/actions/runs/15035119910
If you want, I can raise another PR for Build Windows Packages after it has full functionality. What do you think?

marbre · 2025-05-15T07:58:05Z

@marbre, currently, Build Windows Packages on the main branch does not complete successfully. https://github.com/ROCm/TheRock/actions/runs/15035119910 If you want, I can raise another PR for Build Windows Packages after it has full functionality. What do you think?

It does succeed on the main branch on push, see https://github.com/ROCm/TheRock/actions/runs/15031073217/job/42243505016. However, there is obviously a permission issue when manually dispatching the workflow. This should be fixed with #622 but shouldn't block to add Windows specific adjustments to this PR anyway.

ScottTodd · 2025-05-15T18:50:33Z

Aside: I'm seeing lots of formatting commits pushed individually. If you locally set up pre-commit (or the specific formatters manually), that will let you automatically format your commits before you push.

erman-gurses · 2025-05-15T18:56:28Z

Aside: I'm seeing lots of formatting commits pushed individually. If you locally set up pre-commit (or the specific formatters manually), that will let you automatically format your commits before you push.

Yes, I will figure out automatically call black formatting. locally set up pre-commit sounds more comprehensive - I can also try that - thanks for pointing out.

erman-gurses · 2025-05-15T19:22:53Z

@marbre, currently, Build Windows Packages on the main branch does not complete successfully. https://github.com/ROCm/TheRock/actions/runs/15035119910 If you want, I can raise another PR for Build Windows Packages after it has full functionality. What do you think?

It does succeed on the main branch on push, see https://github.com/ROCm/TheRock/actions/runs/15031073217/job/42243505016. However, there is obviously a permission issue when manually dispatching the workflow. This should be fixed with #622 but shouldn't block to add Windows specific adjustments to this PR anyway.

#622 helps a lot - making progress on Windows side.

ScottTodd

Workflow changes LGTM. Just some Python style comments now.

ScottTodd · 2025-05-16T19:42:34Z

When responding to review feedback, you can batch your commit pushes to avoid re-triggering the CI so many times. Each push starts several hours of build jobs. Pushes to cancel previous jobs, but CI time is not cheap.

erman-gurses · 2025-05-16T19:59:05Z

When responding to review feedback, you can batch your commit pushes to avoid re-triggering the CI so many times. Each push starts several hours of build jobs. Pushes to cancel previous jobs, but CI time is not cheap.

Sure thing I can do that.

erman-gurses · 2025-05-16T21:26:22Z

Final test after the last changes:
Windows: https://github.com/ROCm/TheRock/actions/runs/15076434735
Linux: https://github.com/ROCm/TheRock/actions/runs/15076429933

ScottTodd

Looks good enough to me now. A few lingering comments for future work.

ScottTodd · 2025-05-16T22:41:18Z

+def normalize_path(p: Path) -> str:
+    return str(p).replace("\\", "/") if is_windows() else str(p)


You can probably use as_posix() here instead of defining a new helper function, if the indexer script really needs a unix style path. Fine as-is for now.

Ohh - did not know that.

Instead of forking indexer.py I propose to rather implement a solution based on boto3 which can then be used in an AWS Lambda.

github-project-automation Bot added this to TheRock Triage May 9, 2025

github-project-automation Bot moved this to TODO in TheRock Triage May 9, 2025

erman-gurses requested a review from ScottTodd May 9, 2025 21:48

erman-gurses linked an issue May 9, 2025 that may be closed by this pull request

Upload build logs even if a build target fails (not using teatime.py) #588

Closed

erman-gurses requested a review from marbre May 10, 2025 02:49

ScottTodd requested changes May 12, 2025

View reviewed changes

erman-gurses requested a review from ScottTodd May 12, 2025 16:45

erman-gurses force-pushed the users/erman-gurses/upload-failes-logs-in-workflow branch 2 times, most recently from 5452f72 to 1ec90cd Compare May 13, 2025 01:28

marbre reviewed May 13, 2025

View reviewed changes

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread .github/workflows/build_linux_packages.yml Outdated

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread .github/workflows/build_linux_packages.yml Outdated

erman-gurses requested a review from marbre May 13, 2025 16:46

marbre requested changes May 13, 2025

View reviewed changes

marbre requested changes May 14, 2025

View reviewed changes

Comment thread .github/workflows/build_linux_packages.yml Outdated

Comment thread .github/workflows/build_linux_packages.yml Outdated

ScottTodd requested changes May 14, 2025

View reviewed changes

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread .github/workflows/build_linux_packages.yml Outdated

erman-gurses force-pushed the users/erman-gurses/upload-failes-logs-in-workflow branch from 5c12c12 to 87b107d Compare May 15, 2025 00:49

erman-gurses requested review from ScottTodd and marbre May 15, 2025 01:21

ScottTodd reviewed May 15, 2025

View reviewed changes

Comment thread build_tools/upload_logs_to_s3.py Outdated

Comment thread build_tools/create_log_index.py Outdated

erman-gurses force-pushed the users/erman-gurses/upload-failes-logs-in-workflow branch from 75f2fba to 5a16be1 Compare May 15, 2025 17:50

erman-gurses force-pushed the users/erman-gurses/upload-failes-logs-in-workflow branch 2 times, most recently from bf66621 to c1bc0a1 Compare May 15, 2025 23:10

erman-gurses requested a review from ScottTodd May 16, 2025 02:52

erman-gurses added 9 commits May 16, 2025 10:50

Update the commands for fake logs

2e0c17d

Revert back for cmake original

b286cfc

Revert back for cmake original for linux

3966f7f

Update create log index for windows

da15453

Update Add Links to Job Summary

6fb5913

Convert script type from Windows to Linux on the workflow

0ff4a6d

Add correct path for windows artifacts

69378cf

Temporary add always for Artifacts uploading

41a3b90

Revert always for Artifacts uploading

eacea5b

erman-gurses force-pushed the users/erman-gurses/upload-failes-logs-in-workflow branch from 6b64687 to eacea5b Compare May 16, 2025 17:53

ScottTodd requested changes May 16, 2025

View reviewed changes

erman-gurses added 5 commits May 16, 2025 11:19

Remove local comments

b5f5e92

Update If statement

82f1c48

Update parameter types and logic based on that

dd62589

Keep s3_destination as string

2d3efa4

Add more informative log message with parameter

1df6f36

erman-gurses added 2 commits May 16, 2025 12:53

Add root dir to avoid confusion

1365e59

Update variable name that fits with logic

82c574d

erman-gurses requested a review from ScottTodd May 16, 2025 20:02

ScottTodd approved these changes May 16, 2025

View reviewed changes

Add TODO for indexer.py

98f645e

erman-gurses merged commit c7190c1 into main May 16, 2025
5 checks passed

erman-gurses deleted the users/erman-gurses/upload-failes-logs-in-workflow branch May 16, 2025 23:09

github-project-automation Bot moved this from TODO to Done in TheRock Triage May 16, 2025

erman-gurses mentioned this pull request May 17, 2025

Add log upload support for build target failed using teatime.py script #648

Closed

ScottTodd mentioned this pull request May 27, 2025

Add indexer.py under third-party/indexer/ #717

Merged

ScottTodd mentioned this pull request Aug 4, 2025

[CI] Create ninja logs archive script #1161

Merged

		def normalize_path(p: Path) -> str:
		return str(p).replace("\\", "/") if is_windows() else str(p)

Conversation

erman-gurses commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erman-gurses commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erman-gurses commented May 10, 2025

Uh oh!

Uh oh!

ScottTodd May 12, 2025

Choose a reason for hiding this comment

Uh oh!

erman-gurses May 12, 2025

Choose a reason for hiding this comment

Uh oh!

ScottTodd May 12, 2025

Choose a reason for hiding this comment

Uh oh!

erman-gurses May 12, 2025

Choose a reason for hiding this comment

Uh oh!

erman-gurses May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erman-gurses May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erman-gurses May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erman-gurses commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marbre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marbre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erman-gurses commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marbre commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ScottTodd commented May 15, 2025

Uh oh!

erman-gurses commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erman-gurses commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erman-gurses commented May 9, 2025 •

edited

Loading

erman-gurses commented May 9, 2025 •

edited

Loading

erman-gurses May 12, 2025 •

edited

Loading

erman-gurses May 12, 2025 •

edited

Loading

erman-gurses May 13, 2025 •

edited

Loading

erman-gurses commented May 15, 2025 •

edited

Loading

marbre commented May 15, 2025 •

edited

Loading

erman-gurses commented May 15, 2025 •

edited

Loading

erman-gurses commented May 15, 2025 •

edited

Loading

erman-gurses commented May 16, 2025 •

edited

Loading

erman-gurses commented May 16, 2025 •

edited

Loading