Media Libraries - Python package#3579
Conversation
marbre
left a comment
There was a problem hiding this comment.
I'd suggest to split this up to land native packages and Python packages separately. While the PR title suggests this only adds Python packages, it also adds native Linux packages.
Python packages are failing: https://github.com/ROCm/TheRock/actions/runs/22341297194/job/64655355988?pr=3579
We should also discuss the gating. Other components can also be disabled in the build system but aren't have these kind of checks. I think it could either go into the packages with appropriate defaults set (which makes gating superfluous) or we may want to create a separate package?
@marbre - Updated to reflect the linux and python package additions. The naming and being part of the core was provided by packaging requirements. I have removed the extra checks to match existing method all packages follow. |
@kiritigowda with the above I was referring to add an additional Python package eventually. The extra checks also only affected Python packages and were unrelated to native Linux packages. Please also update the PR description. Splitting might still be the easier path forward as different folks need to review the different parts (the reviewers you originally requested were not involved in Python packaging). Edit: I think we may want to handle this similar like other tools? See 505df93. |
nunnikri
left a comment
There was a problem hiding this comment.
packaging/linux/package.json changes LGTM
|
@marbre -- RFC doesnt talk abt amdrocm-media meta package. So rocdecode and rocjpeg just needs to be part of rocm-core-sdk. https://github.com/ROCm/TheRock/blob/main/docs/rfcs/RFC0009-OS-Packaging-Requirements.md, if there are any other plans for a new package let us know @stellaraccident |
The RFC only refers to native Linux packages and says
@kiritigowda, see my previous comment
I explicitly raised if we should add a new Python package, this is not covered by the RFC. |
@marbre I did create amdrocm-media python package in the original media addition PR, but was asked not to create a new package in the review - #3098 (comment) We are open to creating a new/add to existing, I don't have an issue with either. In this PR we are trying to be consistent with the linux package |
erman-gurses
left a comment
There was a problem hiding this comment.
A couple of suggestions from my side as far as I understand:
-
Please update the PR description to reflect current scope (Python-only; Linux moved to #3614) and explicitly state that these libs are being included in rocm-sdk-core (via core_artifact_filter + _dist_info.py LibraryEntry).
-
Strengthen the test plan with concrete verification steps (e.g., inspect the built rocm-sdk-core wheel contents and confirm librocdecode.so* / librocjpeg.so* are present in expected paths).
Linux packaging updates in PR #3614
You're mixing up things. The RFC as well as the comment you're linking to is for native Linux packages. My question is about Python packages.
Python packages and native Linux packages are not consistent anyway as we for now only have |
## Motivation Split PR #3579 into linux and python -- This PR contains linux packaging implementation ## Technical Details Media linux packages ## Test Plan Current tests ## Test Result No current tests should fail ## Submission Checklist - [x ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
@marbre I agree with you, the core libraries can not be optional. We need to have a discussion and figure this out. Let me know if you have any directions or discuss this in the office hours? |
There was a problem hiding this comment.
this pr still seems to mix with #3098 where you now decided to introduce a new python packages rocm-media? was there already some discussion?
maybe you can also explain more why media libs should be an optional requirement - compared to the other components we are having.
please also update your pr description with more context - as this should be also a helpful message for people not familiar with media libs. e.g. it would be good to also reference the other pr for native packaging (and match titles) and the pr where the media libs were introduced.
and some testing will be needed to verify that the python wheels include correctly the media libs and they can be used.
please untangle this pr and #3098 so that one can better review those prs.
edit: fixed link to correct pr
@erman-gurses -- added the comments to the description. The tests to verify the install is a good step, we can add that as an enhancement. (There is no current framework to test any libs in python package in the CI) |
@HereThereBeDragons The PR #3098 was the original PR with the full integration of media libraries, we took the reviewer comment and split this PR into multiple PRs. Currently this PR #3098 only has test inclusion. @marbre - For python packaging on the rock, the media libraries are always built by design, it's only optional for end-user. The media libraries (rocdecode and rocjpeg) are being added to the core sdk to match the linux packaging. For now we want this to be included as core sdk. |
HereThereBeDragons
left a comment
There was a problem hiding this comment.
@kiritigowda from your discussion i still dont understand the final decision:
- put media into a separate python packages (which the other pr does)
- or like this PR: have it in rocm-core
if it is in rocm-core you cannot have it as a conditional logic that it isnt always added to the package.
if you are unsure which is the best approach, then please bring up the topic during the office hours or maybe @saadrahim has some preference?
@HereThereBeDragons - the media libs are included in core sdk for linux - In this PR we are being consistent. |
Motivation
Python packaging for media libraries (rocdecode & rocjpeg)
Technical Details
Libs are being included in rocm-sdk-core (via core_artifact_filter + _dist_info.py LibraryEntry)
Test Plan
Verify if media packages are generated
Test Result
No current tests should fail
Submission Checklist