Skip to content

script to verify installation#3124

Closed
jonatluu wants to merge 2 commits into
ROCm:mainfrom
jonatluu:jonatluu/install_test
Closed

script to verify installation#3124
jonatluu wants to merge 2 commits into
ROCm:mainfrom
jonatluu:jonatluu/install_test

Conversation

@jonatluu
Copy link
Copy Markdown
Contributor

Motivation

Script which verifies packages can be installed

Technical Details

python script installs packages using apt

Test Plan

test install and uninstall for deb and rpm

Test Result

Submission Checklist

Copy link
Copy Markdown
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

There are some design and logic issues here. Furthermore the PR title and description is not appropriate. This PR is not limited to adding a script and rather aims to "Add testing for native packages" as it also touched the workflow. Furthermore, pre-submit is failing and there are no test results linked.

Take a look into how we test ROCm wheels and especially PyTorch wheels. Testing should go to a separate job. @HereThereBeDragons can give further guidance. Will review again once this was addressed and Laura's reviewed.

--package-type rpm \
--package-folder ${{ env.PACKAGE_DIST_DIR }} \
--docker-image ${{ env.BUILD_IMAGE }}
fi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has some design flaws from my perspective.

  1. Testing should probably be run in a separate job and should depend on building. This would allow to run the job in a container. Furthermore, this it would be preferred to use a matrix to spawn different distributions.
  2. The job always run ins a manylinux docker container. Spawning a manylinux container in a manylinux container is not what we want and it should rather match on of the OS profiles.
  3. Deb testing wont work if installing in the manylinux container. Instead an Ubuntu or Debian container needs to be spawned.

@HereThereBeDragons can give some guidance here as some of the designed are most likely used / considered in the upcoming weekly CI.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is pretty large for just a simple test install which could be a narrowed down to 2-3 lines. Printing should be avoided and replaced by proper logging. Following the above comments I also don't see the need to start the container within the script. Haven't looked closer thus I assume there is more to fix.

@marbre marbre mentioned this pull request Jan 28, 2026
3 tasks
@jonatluu jonatluu closed this Jan 30, 2026
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Jan 30, 2026
radhaksri added a commit that referenced this pull request Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants