Bug 2221418 - Review Request: python-tensile - Tool for creating benchmark-driven backend libraries for GEMMs
Summary: Review Request: python-tensile - Tool for creating benchmark-driven backend l...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tom Rix
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-07-08 22:45 UTC by Jeremy Newton
Modified: 2024-01-11 01:08 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2024-01-11 01:08:38 UTC
Type: ---
Embargoed:
trix: fedora-review+


Attachments (Terms of Use)

Description Jeremy Newton 2023-07-08 22:45:24 UTC
Spec URL: https://mystro256.fedorapeople.org/python-tensile.spec
SRPM URL: https://mystro256.fedorapeople.org/python-tensile-5.6.0-1.fc39.src.rpm
COPR Build: https://copr.fedorainfracloud.org/coprs/mystro256/rocm-hip/build/6154053/
Description:
Tensile is a tool for creating benchmark-driven backend libraries for GEMMs,
GEMM-like problems (such as batched GEMM), and general N-dimensional tensor
contractions on a GPU. The Tensile library is mainly used as backend library to
rocBLAS. Tensile acts as the performance backbone for a wide variety of
'compute' applications running on AMD GPUs.
Fedora Account System Username: mystro256

Comment 1 Jeremy Newton 2023-07-08 22:48:09 UTC
Note that I'm still working on rocblas, but it appears to find tensile fine as-is.
rocBLAS uses the devel files for some stuff, so I just trimmed things down a bit by removing the miopen archives.

Comment 2 Jeremy Newton 2023-07-08 23:00:56 UTC
I think I might need to add rocm-smi-devel has a requires for the devel package.
Tensile is a bit odd, its a python lib but installs some source code for rocblas to consume.

Comment 3 Tom Rix 2023-07-09 15:38:26 UTC
The _bindir/* should work, this is the first one i tried

fedora > /usr/bin/tensile_rocblas_cgemm 
...
b'CMake Error: The source directory "/usr/lib/python3.12/site-packages/Tensile/Source" does not exist.\nSpecify --help for usage, or press the help button on the CMake GUI.\n'
Traceback (most recent call last):
  File "/usr/bin/tensile_rocblas_cgemm", line 8, in <module>
    sys.exit(TensileROCBLASCGEMM())

...

subprocess.CalledProcessError: Command '['cmake', '-DCMAKE_BUILD_TYPE=Release', '-DTENSILE_USE_MSGPACK=ON', '-DTENSILE_USE_LLVM=ON', '-DTensile_LIBRARY_FORMAT=msgpack', '-DCMAKE_CXX_COMPILER=/opt/rocm/bin/hipcc', '-DCMAKE_C_COMPILER=/opt/rocm/bin/hipcc', '/usr/lib/python3.12/site-packages/Tensile/Source']' returned non-zero exit status 1.

So at least /Source needs be in the python3-tensile 
The use /opt/rocm/bin/hipcc needs to change /usr/bin/hipcc or just hipcc and assume PATH will find it

why the license and readme needed in the -devel package ?

i am not sure about the miopen configs either, miopen needs blas and blas need this..
let's not over think it and add them back if there is a problem.

check is an option, but Tests/ is always installed, should installing Tests/ follow the check option ?

Comment 4 Jeremy Newton 2023-07-09 17:52:29 UTC
> So at least /Source needs be in the python3-tensile 

My gut says that everything should be in the devel package, as it seems like tensile is a python tool inter-tangled with a static library.

> The use /opt/rocm/bin/hipcc needs to change /usr/bin/hipcc or just hipcc and assume PATH will find it

Ack, I should probably look to see what Debian did, so I can reduce some back and forth.

> why the license and readme needed in the -devel package ?

It was a mistake, I'll fix it.

> i am not sure about the miopen configs either, miopen needs blas and blas need this..
> let's not over think it and add them back if there is a problem.

Yeah I can just drop the "rm" later. It's mostly duplicated files, so I'd rather exclude it unless it's clear we need it.

> check is an option, but Tests/ is always installed, should installing Tests/ follow the check option ?

I'm not sure if the tests are used for rocblas tests yet. In the meantime, I can do the same as the miopen archives and remove it for now.

Comment 5 Tom Rix 2024-01-07 16:54:13 UTC
Spec URL: https://trix.fedorapeople.org/python-tensile.spec
SRPM URL: https://trix.fedorapeople.org/python-tensile-6.0.0-1.fc40.src.rpm

Many warning are like
python3-tensile.noarch: W: devel-file-in-non-devel-package /usr/lib/python3.12/site-packages/Tensile/Source/lib/include/Tensile/ocl/OclUtils.hpp       
python3-tensile.noarch: W: devel-file-in-non-devel-package /usr/lib/python3.12/site-packages/Tensile/Source/lib/source/AMDGPU.cpp 

And i guess why there was originally a -devel subpackage.
But these are not headers to a library but rather inputs to the tensile code generating.
So I dropped the -devel subpackage

%check works a little better but not well enough to be a default.
%pytest works better than %tox, but there are still a lot of errors like 'can not find builddir' 

The patches fix the usages of /opt/rocm that i ran into.

licensecheck.txt did not report any BSD so I dropped it from %license

Works well enough to build rocblas locally and i am working through using it in the rocm package package

If you like I could take over the submission of Tensile and we could swap roles.

Comment 6 Jeremy Newton 2024-01-08 18:48:36 UTC
I'm pretty ok with what you did. If we're allowed to swap roles, feel free. I can review it if that's the case.

Comment 7 Jeremy Newton 2024-01-08 19:39:53 UTC
After a quick skim:

Should we include %{python3_sitelib}/%{upstreamname}/Tests in a test package? (e.g. you can guard it with %if %{with check}, like you did with the other rocm packages)

Also you need to drop:
> %package -n python3-tensile-devel
> Summary:        The python3-tensile development package
> Requires:       %{name} = %{version}-%{release}
> 
> %description -n python3-tensile-devel
> The python3-tensile development package.

If you're dropping that package.

Comment 8 Jeremy Newton 2024-01-08 19:46:44 UTC
also it might be better to convert those patches into sed commands in %prep to reduce some fragility to updates.

It's pretty straightforward:

sed -i 's|/opt/rocm|%{_prefix}|' Tensile/Common.py
sed -i 's|/opt/rocm|%{_prefix}|' Tensile/Tests/yaml_only/test_config.py
sed -i 's|${Tensile_PREFIX}|%{_prefix}|' Tensile/cmake/TensileConfig.cmake

Comment 9 Tom Rix 2024-01-10 17:17:53 UTC
Spec URL: https://trix.fedorapeople.org/python-tensile.spec
SRPM URL: https://trix.fedorapeople.org/python-tensile-6.0.0-2.fc40.src.rpm

Here's an update with the changes.

Comment 10 Jeremy Newton 2024-01-10 21:37:17 UTC
Ok so for reference, the review checklist is here:
https://download.copr.fedorainfracloud.org/results/mystro256/playground/fedora-rawhide-x86_64/06880168-python-tensile/fedora-review/review.txt

I looked through it and I think the only outstanding issue is:
>      Note: Directories without known owners: /usr/share/cmake
This is because the package MUST require on cmake-filesystem to avoid causing the directory becoming orphaned after uninstall.
So please add "Requires: cmake-filesystem" (sorry I'm a stickler for correct filesystem ownership)

As well to be clear, you've already confirmed that files in /usr/lib/python3.12/site-packages/Tensile/Source/ are not devel files.

With that I'll add my approval, but please don't forget the extra cmake-filesystem requires.

Comment 11 Fedora Admin user for bugzilla script actions 2024-01-10 22:18:26 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-tensile


Note You need to log in before you can comment on or make changes to this bug.