Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22094 closed defect (fixed)

setup.py: run_autogen is ran too late

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-7.5
Component: build Keywords:
Cc: egourgoulhon, embray, fbissey Merged in:
Authors: François Bissey Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 8e172f6 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

When building Sage from scratch, the following happens:

ImportError: No module named interpreters.wrapper_rdf

In src/setup.py, the method run_autogen is ran after discovering and building the Python modules. So, the Python module sage/ext/interpreters/__init__.py is not installed.

This problem was introduced in #21613.

Change History (22)

comment:1 Changed 2 years ago by jdemeyer

  • Cc fbissey added

comment:2 Changed 2 years ago by jdemeyer

Untested hypothesis: the "Discovering Python/Cython source code" part below is run before auto-generating the sources, which is the wrong order:

print("Discovering Python/Cython source code....")
t = time.time()
from sage_setup.find import find_python_sources
python_packages, python_modules = find_python_sources(
    SAGE_SRC, ['sage', 'sage_setup'])

The auto-generated files might not be discovered this way, leading to those files being considered for cleaning.

comment:3 Changed 2 years ago by fbissey

OK I can reproduce it at will here by taking a full build and then doing MAKE="make -j8" ./sage -ba (not sure if it fails with a single threaded build). ./sage will crash and complain about interpreters.wrapper_rdf in the crash report.

comment:4 follow-up: Changed 2 years ago by jdemeyer

I just reproduced the problem by building Sage from scratch (make distclean; make). I thought the buildbots would test that? In the process, I found another problem: #22098.

comment:5 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from ImportError: No module named interpreters.wrapper_rdf to setup.py: run_autogen is ran too late

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by egourgoulhon

Here are some experiments regarding the reproductibility on my system (Ubuntu 16.04):

  • the error seems systematic: i.e. running MAKE="make -j8" make in a new fresh git clone yields the same error; then running make finishes the build without any error
  • to determine whether this due to the parallel build, I ran simply make in another fresh git clone: then the build crashes due to another error: the psutil one reported in #22098

comment:7 in reply to: ↑ 6 ; follow-up: Changed 2 years ago by egourgoulhon

With the correction proposed in #22098, the non-parallelized build with make goes further but halts at the same error as reported in the current ticket, i.e.

[dochtml]   File "sage/ext/interpreters/wrapper_rdf.pxd", line 7, in init sage.plot.plot3d.parametric_surface (/home/eric/sage/7.5.rc0-22098/src/build/cythonized/sage/plot/plot3d/parametric_surface.c:12253)
[dochtml] ImportError: No module named interpreters.wrapper_rdf

So clearly the issue is not due to the parallel build.

comment:8 in reply to: ↑ 7 Changed 2 years ago by egourgoulhon

Replying to egourgoulhon:

So clearly the issue is not due to the parallel build.

As for the parallel build, running make a second time leads to a successful build. In other words, make && make works, while a single make fails...

comment:9 Changed 2 years ago by vbraun

The buildbot tests building from scratch but for some reasons succeeds... there is most likely a random component to it.

comment:10 Changed 2 years ago by fbissey

On the machine it fails, it looks quite systematic. Something in the environment?

comment:11 Changed 2 years ago by jdemeyer

I think it's pointless to figure out the exact circumstances when the problem occurs. I analysed it well enough (see ticket description) that somebody could fix it.

comment:12 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 2 years ago by fbissey

  • Authors set to François Bissey
  • Branch set to u/fbissey/wrapper
  • Commit set to 6c2a4caaaeea3b6082936d57c48e8a949ebe3e00
  • Status changed from new to needs_review

OK, this branch works for me but I don't know if it is the most appropriate way to do it. Needs review.


New commits:

6c2a4caMove autogen to a more appropriate location.

comment:14 follow-up: Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

It's not "Generating Interpreter Sources", it's "Generating auto-generated sources". So both the comment and the print statement are wrong in this regard.

About the actual solution: I'm pretty sure that Erik won't like it (in setup.py, you are supposed to only do stuff within the setup() call). However, this fixes a blocker issue, so I am OK with this branch being a stopgap.

comment:15 in reply to: ↑ 14 Changed 2 years ago by fbissey

Replying to jdemeyer:

It's not "Generating Interpreter Sources", it's "Generating auto-generated sources". So both the comment and the print statement are wrong in this regard.

Amusing considering I copied pasted the original print statement from Erik. I can see it was more general before #21613.

About the actual solution: I'm pretty sure that Erik won't like it (in setup.py, you are supposed to only do stuff within the setup() call). However, this fixes a blocker issue, so I am OK with this branch being a stopgap.

OK for the stopgap status. If Erik, think it can be done better he is welcome to do it, but it is the last blocker for 7.5 so getting things out is important. New commit coming soon.

comment:16 Changed 2 years ago by git

  • Commit changed from 6c2a4caaaeea3b6082936d57c48e8a949ebe3e00 to 8e172f646e284ed98b3c0d57dd0a43792084efcc

Branch pushed to git repo; I updated commit sha1. New commits:

8e172f6amending coment and message in setup.py

comment:17 Changed 2 years ago by fbissey

  • Status changed from needs_work to needs_review

I also forgotten, I wanted to include the ticket number in the comment, so that was a good opportunity.

comment:18 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:19 Changed 2 years ago by egourgoulhon

Just to tell that François' commit fixes the build from scratch for me too.

comment:20 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 2 years ago by vbraun

  • Branch changed from u/fbissey/wrapper to 8e172f646e284ed98b3c0d57dd0a43792084efcc
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 2 years ago by mkoeppe

  • Commit 8e172f646e284ed98b3c0d57dd0a43792084efcc deleted

Follow-up: #22106

Note: See TracTickets for help on using tickets.