Bug 1975046 - Review Request: python-aesara - Python optimizing compiler for evaluating mathematical expressions
Summary: Review Request: python-aesara - Python optimizing compiler for evaluating mat...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2021-06-23 02:36 UTC by Sergio Pascual
Modified: 2022-01-19 00:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-01-19 00:45:13 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Sergio Pascual 2021-06-23 02:36:53 UTC
Spec URL: https://sergiopr.fedorapeople.org/python-aesara.spec
SRPM URL: https://sergiopr.fedorapeople.org/python-aesara-2.0.10-1.fc35.src.rpm
Description: Aesara is a Python library that allows you to define, optimize, and 
efficiently evaluate mathematical expressions involving multi-dimensional 
arrays. It is built on top of NumPy. (This is a fork of theano)
Fedora Account System Username: sergiopr

Comment 2 Arthur Bols 2021-06-23 11:28:23 UTC
Hi,

Please note that this a preliminary review.

I checked the license situation (and reread the licensing guidelines) and I think it should be fine.

rpmlint reports errors for check_blas.py and check_multi_gpu.py even though you tried to fix it :/ 
  
  python3-aesara.noarch: E: non-executable-script /usr/lib/python3.10/site-packages/aesara/misc/check_blas.py 644 /usr/bin/env python
  python3-aesara.noarch: E: non-executable-script /usr/lib/python3.10/site-packages/aesara/misc/check_multi_gpu.py 644 /usr/bin/env python

Would it be better to use the sources from github? This way you could include the tests and create a *-doc package.

Some small suggestions:
- Change pytest to the %pytest macro.
- Change python3-setuptools to python3dist(setuptools)

Comment 3 Robert-André Mauchin 🐧 2021-07-04 08:08:39 UTC
I'll finish the review started by Arthur, please take into account his review and post a new spec.

Comment 4 Ben Beasley 2021-07-15 02:51:21 UTC
Setting NEEDINFO based on Robert-André’s request.

Comment 5 Sergio Pascual 2021-07-19 00:15:29 UTC
New upstream source 2.1.2

Now spec includes a smoke test by importing aesara. Upstream doesn't include test in its pypi tarball

flexiblas and the c++ compiler are requires, the package creates and compiles C code

Spec URL: https://sergiopr.fedorapeople.org/python-aesara.spec
SRPM URL: https://sergiopr.fedorapeople.org/python-aesara-2.1.2-1.fc35.src.rpm

Comment 6 Ben Beasley 2021-11-29 14:28:26 UTC
Robert-André, are you still planning to do this review?

Comment 7 Robert-André Mauchin 🐧 2021-12-19 21:12:33 UTC
I don't really have time for review these days.

> No tests in the tarball from PyPI

Use the github tarball in that case.

The Python packaging guidelines have changed, here a sample spec with test deactivated for now

# Disabled by default
%bcond_with check

%global pypi_name aesara
%global fullversion rel-%{version}

Name:           python-%{pypi_name}
Version:        2.3.3
Release:        %autorelease
Summary:        Optimizing compiler for evaluating mathematical expressions on CPUs and GPUs

# These files are under MIT
# aesara/graph/sched.py
# aesara/misc/ordered_set.py
# aesara/scalar/c_code/Faddeeva.cc
# aesara/scalar/c_code/Faddeeva.hh
License:        BSD and MIT
URL:            https://github.com/aesara-devs/aesara
Source0:        %{url}/archive/%{fullversion}/%{pypi_name}-%{fullversion}.tar.gz

BuildArch:      noarch

BuildRequires:  python3-devel

%global _description %{expand:
Aesara is a Python library that allows you to define, optimize, and
efficiently evaluate mathematical expressions involving multi-dimensional
arrays. It is built on top of NumPy.}

%description %_description

%package -n     python3-%{pypi_name}
Summary:        %{summary}

Requires:       gcc-c++
Requires:       flexiblas-devel
Recommends:     %{py3_dist pygpu}

%description -n python3-%{pypi_name} %_description

%prep
%autosetup -p1 -n %{pypi_name}-%{fullversion}
# Remove bundled egg-info
rm -rf %{pypi_name}.egg-info

%generate_buildrequires
%pyproject_buildrequires
%if %{with check}
echo 'gcc-c++'
echo 'flexiblas-devel'
echo '%{py3_dist pytest}'
echo '%{py3_dist numpy}'
echo '%{py3_dist filelock}'
echo '%{py3_dist scipy}'
echo '%{py3_dist pygpu}'
echo '%{py3_dist cons}'
echo '%{py3_dist etuples}'
echo '%{py3_dist logical-unification}'
echo '%{py3_dist minikanren}'
%endif

%build
%pyproject_wheel

%install
%pyproject_install
rm -rvf %{buildroot}%{python3_sitelib}/bin
%pyproject_save_files %{pypi_name}

%if %{with check}
%check
%pytest
%endif

%files -n python3-%{pypi_name} -f %{pyproject_files}
%license LICENSE.txt doc/LICENSE.txt
%doc README.rst
%{_bindir}/aesara-cache

%changelog
%autochangelog



You have missing dependencies to install the package correctly:


Error: 
 Problem: conflicting requests
  - nothing provides python3.10dist(cons) needed by python3-aesara-2.3.3-2.fc36.noarch
  - nothing provides python3.10dist(etuples) needed by python3-aesara-2.3.3-2.fc36.noarch
  - nothing provides python3.10dist(logical-unification) needed by python3-aesara-2.3.3-2.fc36.noarch
  - nothing provides python3.10dist(minikanren) needed by python3-aesara-2.3.3-2.fc36.noarch

Comment 8 Ben Beasley 2021-12-20 01:21:32 UTC
(In reply to Robert-André Mauchin 🐧 from comment #7)
> %generate_buildrequires
> %pyproject_buildrequires
> %if %{with check}
> echo 'gcc-c++'
> echo 'flexiblas-devel'
> echo '%{py3_dist pytest}'
> echo '%{py3_dist numpy}'
> echo '%{py3_dist filelock}'
> echo '%{py3_dist scipy}'
> echo '%{py3_dist pygpu}'
> echo '%{py3_dist cons}'
> echo '%{py3_dist etuples}'
> echo '%{py3_dist logical-unification}'
> echo '%{py3_dist minikanren}'
> %endif

This works, but (1) most of these are in the install_requires and will be generated if you use “%pyproject_buildrequires -r”, and (2) it works perfectly well to mix “traditional” and generated BuildRequires, so “BuildRequires:  gcc-c++” and so on in the usual part of the spec file would work equally well, and perhaps be a little less surprising.

As you mentioned, several of these dependencies need to be packaged.

I haven’t really attempted to review the existing submission, but I did also notice that aesara/d3viz is teeming with bundled, pre-minified CSS and JavaScript. It’s increasingly hard to package web assets at all given conflicts between modern build pipelines and the guidelines in https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/ and https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/; it may end up being necessary to simply try to remove this part of the library.

Comment 9 Package Review 2022-01-19 00:45:13 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


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