Conversation
859f2ef to
57593b8
Compare
57593b8 to
b4a0d5a
Compare
b4a0d5a to
4d9a212
Compare
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
b549d6e to
9639199
Compare
Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
bef95b3 to
5618d07
Compare
- Link amdsminic static library into libamd_smi_python.so to resolve undefined NIC symbols - Prevent pip-context wrapper from falling back to system libamd_smi.so to avoid dual-loading segfault - Add trailing newline to setup.cfg.in
5618d07 to
87e9051
Compare
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
ed79383 to
68aa824
Compare
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
68aa824 to
b0dc79f
Compare
| except Exception: | ||
| _log_version_and_path_diagnostics() | ||
| print("[amdsmi-cli] Failed to read amdsmi Python package version; aborting to avoid incompatibility.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
This is where my code is failing to run in the container. Once we get this fixed I will approve as the rest looks great
| class struct_amdsmi_pcie_info_t(Structure): | ||
| pass | ||
|
|
||
| class struct_pcie_static_(Structure): |
There was a problem hiding this comment.
This is unrelated to this PR, did you have a stray commit in here? I think we'd want to have this separate.
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
00aaf6a to
83dea0d
Compare
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
There was a problem hiding this comment.
I took a brief look myself and furthermore let an agent scan through this. As pointed out earlier this will need a written-down RFC before going further with the implementation. Here are some of the comments my agent left:
Python Wheel Version Tagging
The package has no C extension modules — no ext_modules, no cython, no cffi. libamd_smi_python.so is a C++ shared library bundled as package_data, not as a Python extension. pyproject.toml.in and setup.cfg.in both confirm this. Setuptools builds packages with no C extensions as py3-none-linux_x86_64 — Python version agnostic.
The manylinux workflow loops over six interpreters (cp38–cp313), but because the wheel tag is py3-none-linux_x86_64 for all of them, each iteration produces a file with the same name. Successive builds overwrite each other in $RAW_WHEELS. The "six wheels" outcome advertised by the workflow is not what actually happens.
auditwheel was designed for ABI-tagged wheels (cpXX-cpXX-linux_x86_64). Its behavior on py3-none-linux_x86_64 wheels that bundle .so data files is not well-defined and may produce incorrect tags, refuse the input, or pass it through unchanged.
Metadata Duplication:
Both files setup.cfg.in and pyproject.toml.in exist on the branch and both declare the full package metadata (name, version, author, classifiers, etc.). With setuptools ≥ 61, pyproject.toml takes precedence, but having two authoritative metadata sources creates a maintenance hazard: a version bump that updates one but not the other silently produces stale metadata.
CI / Workflow
-
rpm-testnowneeds: [manylinux-build, debian-test]. This creates a cross-platform serialization: RPM tests cannot start until all Debian matrix legs complete (Ubuntu20,Ubuntu22,Ubuntu24,Debian10). Previously the RPM build was independent. Withcontinue-on-error: trueon Debian legs this may not block execution, but it increases total wall time. -
Manylinux gate hardcodes
/opt/python/cp38-cp38/bin/python3. If manylinux_2_28 drops cp38 support in a future image update, the gate permanently breaks. -
chmod 777 "$WHEEL_OUT"inmanylinux-build.ymlis overly broad for a container build artifact directory.
.github/workflows/amdsmi-build.yml
Outdated
| done | ||
|
|
||
|
|
||
| - name: Verify Wheel in Site-Packages |
There was a problem hiding this comment.
What is the reason for not moving this to a dedicated scripts?
| echo "Displaying Unit Test Results for ${{ matrix.os }}" | ||
| cat /tmp/test-results-${{ matrix.os }}/unit_test_output.txt || echo "No unit test results found for ${{ matrix.os }}" | ||
|
|
||
| - name: Perf Test Results |
There was a problem hiding this comment.
Why cat perf test results here instead of uploading them to a more accessible location?
| class struct_amdsmi_pcie_info_t(Structure): | ||
| pass | ||
|
|
||
| class struct_pcie_static_(Structure): |
|
As an additional note, I don't quite follow why you try to leverage https://docs.python.org/3/library/site.html
Rather see https://www.debian.org/doc/packaging-manuals/python-policy/#module-path. Debian use this even though it is not standard, check how RPM-based distros handle it. As above, this needs a written down plan. |
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
379bccb to
6d00c1a
Compare
Signed-off-by: Justin Williams <Justin.Williams@amd.com>
0f80d99 to
4b1c266
Compare
No description provided.