Opened 3 years ago

Closed 2 years ago

#20730 closed task (fixed)

Break up sage_setup.autogen.interpreters into a package

Reported by: embray Owned by:
Priority: major Milestone: sage-8.0
Component: fast_callable Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: b34d5d8 (Commits) Commit: b34d5d8fc509f6773fe3c72c3d1b0e24a358efde
Dependencies: #20238 Stopgaps:

Description

sage_setup.autogen.interpreters was getting a bit long (over 4000 lines) and it was hard to understand how some of it was organized. So I split it up into several logical modules. This seemed to have some support.

How exactly I chose to break it up is certainly up for debate, but I thought this was fairly logical to start. In the __init__.py I still import all from all submodules so that the API for the sage_setup.autogen.interpreters module is unchanged.

I would like to get some version of this accepted soon, as I have a few tasks to work on in this module and it would be easier (though not necessary) to work on it on top of this change.

Change History (57)

comment:1 follow-up: Changed 3 years ago by jdemeyer

I suggest to add from __future__ import absolute_import, print_function to all those modules.

comment:2 in reply to: ↑ 1 Changed 3 years ago by embray

Replying to jdemeyer:

I suggest to add from __future__ import absolute_import, print_function to all those modules.

Funny, I actually did that at first but then took it out since it wasn't strictly needed. No problem adding them back in though.

comment:3 Changed 3 years ago by embray

  • Status changed from new to needs_review

Not sure what component to put this under. There isn't one for fast_callable.

comment:4 Changed 3 years ago by git

  • Commit changed from 2e76f57321b6be633788af6172645b397ca3d0fd to 1a434e8a269cc7eb901817ae0dbd74dd9b804cab

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

1a434e8Add from __future__ import print_function, absolute_import to all (non-empty) modules for forward compatibility.

comment:5 Changed 3 years ago by vdelecroix

  • Component changed from PLEASE CHANGE to fast_callable

Now there is ;-)

comment:6 follow-up: Changed 3 years ago by vdelecroix

Trying to play with your branch I got

[install] [sagelib-7.3.beta2] make[3]: Entering directory '/opt/sage/src'
[install] [sagelib-7.3.beta2] make[3]: warning: -jN forced in submake: disabling jobserver mode.
[install] [sagelib-7.3.beta2] make[3]: *** No rule to make target 'sage_setup/autogen/interpreters.py', needed by 'sage/ext/interpreters/__init__.py'.  Stop.
[install] [sagelib-7.3.beta2] make[3]: Leaving directory '/opt/sage/src'

comment:7 in reply to: ↑ 6 Changed 3 years ago by embray

Replying to vdelecroix:

Trying to play with your branch I got

[install] [sagelib-7.3.beta2] make[3]: Entering directory '/opt/sage/src'
[install] [sagelib-7.3.beta2] make[3]: warning: -jN forced in submake: disabling jobserver mode.
[install] [sagelib-7.3.beta2] make[3]: *** No rule to make target 'sage_setup/autogen/interpreters.py', needed by 'sage/ext/interpreters/__init__.py'.  Stop.
[install] [sagelib-7.3.beta2] make[3]: Leaving directory '/opt/sage/src'

Thanks. I don't run make that often for building sagelib (as opposed to just running setup.py directly) so I didn't catch this.

comment:8 Changed 3 years ago by git

  • Commit changed from 1a434e8a269cc7eb901817ae0dbd74dd9b804cab to 6dbab6144024130f3edd0c29b421f9bf21347d9d

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

3f0541dChange the name attributes of the interpreter implementations to class
0becca2Update Makefile to more explicitly make targets for the files generated for the interpreters, as well as rebuild those targets when any of the autogen/interpreters sources change.
a7c88edMake Makefile simpler to understand / more robust by calling a function in sage_setup.autogen.interpreters that explicitly prints the names of all interpreters.
6dbab61Also avoid reexpansion of the $(INTERP_SRCS) variable.

comment:9 Changed 3 years ago by embray

That should fix it.

comment:10 Changed 3 years ago by jdemeyer

Why did you make src/Makefile so complicated? I find it hard to understand what you're doing there.

comment:11 follow-up: Changed 3 years ago by embray

Which part is hard to understand? The most important change is that since there is not a single interpreters.py, if any file in the sage.autogen.inteprreters package is changed the interpreter sources should be regenerated. Even that's a rough approximation :)

comment:12 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

Replying to embray:

if any file in the sage.autogen.intepreters package is changed the interpreter sources should be regenerated.

That can be done with a single wildcard like we already do for the PARI stuff:

sage/libs/pari/auto_gen.pxi: $(SAGE_LOCAL)/share/pari/pari.desc \
        sage/libs/pari/decl.pxi sage_setup/autogen/pari/*.py
    python -c "from sage_setup.autogen.pari import rebuild; rebuild()"

comment:13 Changed 3 years ago by embray

The current layout I came up with has a sage_setup.autogen.interpreters.specs subpackage, so a single wildcard would not work.

I could do away with the subpackage and that's arguable. I think it makes it easier to find the implementations for the individual interpreter specs, rather than intermingling them with support modules.

comment:14 Changed 3 years ago by embray

One thing I could do that would definitely simplify this a bit would be instead of the interpreters.print_names() function I added, the interpreters module could (and probably should) have a function that returns the names of all the files it generates. That way this wouldn't have to be duplicated in the Makefile, and it would be made simpler.

comment:15 Changed 3 years ago by jdemeyer

Even with the subdirectory you could still use wildcards...

Anyway, there should be a solution which doesn't involve 20 new lines added to src/Makefile.

comment:16 Changed 3 years ago by embray

Well it's a difference of one find or two wildcard patterns. I suppose the latter has the slight advantage of being more explicit. I could try it and see how it looks.

comment:17 Changed 3 years ago by git

  • Commit changed from 6dbab6144024130f3edd0c29b421f9bf21347d9d to 65a05199a4422491e97934569f950cf8ae10ff39

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

301fbe8Deduplicate code in anticipation of adding more generated file(s) for interpreters.
65a0519Simplify Makefile further by replacing print_names() with a list_sources() that just prints the names of all generated source files.

comment:18 Changed 3 years ago by embray

Simplified the Makefile a smidge. It's actually better this way than I had it before, because now sage_setup.autogen.interpreters alone is responsible for knowing the names of the files it outputs, reducing the need to keep track of that information in multiple places.

comment:19 Changed 3 years ago by git

  • Commit changed from 65a05199a4422491e97934569f950cf8ae10ff39 to d5225c350c9b7e01708f68dddff955857a9aea5d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d5225c3Simplify Makefile further by replacing print_names() with a list_sources() that just prints the names of all generated source files.

comment:20 Changed 3 years ago by git

  • Commit changed from d5225c350c9b7e01708f68dddff955857a9aea5d to 206138aaf56066e5cac608a792590e45b82c20f3

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

750a67dAdd all of sage_setup to in_lib so that the doctest runner will not try to load the modules individually.
7621549Add from __future__ import print_function, absolute_import to all (non-empty) modules for forward compatibility.
4570ef2Change the name attributes of the interpreter implementations to class
e73f68eUpdate Makefile to more explicitly make targets for the files generated for the interpreters, as well as rebuild those targets when any of the autogen/interpreters sources change.
22a4ef5Make Makefile simpler to understand / more robust by calling a function in sage_setup.autogen.interpreters that explicitly prints the names of all interpreters.
ffa16f4Also avoid reexpansion of the $(INTERP_SRCS) variable.
1d4dc46Deduplicate code in anticipation of adding more generated file(s) for interpreters.
baa34ebSimplify Makefile further by replacing print_names() with a list_sources() that just prints the names of all generated source files.
9f9c44dFixes to testing sage_setup
206138aEnsure that the value of in_lib is a bool.

comment:21 Changed 3 years ago by embray

Rebased again (incorporating some small Python3-compat fixes that were made in the meantime) and fixed some issues that were causing some tests to fail in the sage_setup package.

comment:22 Changed 3 years ago by tscrim

Jeroen, are you going to be finishing the review of this ticket?

comment:23 Changed 3 years ago by jdemeyer

We should have talked about this ticket this week. I know I'm repeating myself, but I still don't like what you did to src/Makefile.

Anyway, I'll look at the rest.

comment:24 follow-up: Changed 3 years ago by jdemeyer

What's the point of this:

# nodoctest -- __main__ modules are just for running the module from the
# command line and should not be doctested

It doesn't matter since the file doesn't have doctests anyway.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

What's the point of this:

# nodoctest -- __main__ modules are just for running the module from the
# command line and should not be doctested

It doesn't matter since the file doesn't have doctests anyway.

If it didn't matter I wouldn't have even thought to put such a thing. The doctest runner still imports a module to determine if there are any doctests in the module. Since everything in a __main__ module is is executed upon import, we don't want the doctest runner to even import it (there is no if __name__ == '__main__' equivalent since it's a tautology in this case).

comment:26 Changed 3 years ago by mkoeppe

I agree with Jeroen (comment 23) that these Makefile tricks seem unnecessarily complicated. Just the list the few files that are there one by one in the Makefile.

By the way, #21480 puts this interpreter source generation business under control of setup.sh. So there would be an option to do these kind of things in Python, rather than in Makefile.

Last edited 3 years ago by mkoeppe (previous) (diff)

comment:27 Changed 3 years ago by mkoeppe

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to needs_work

comment:28 follow-up: Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Sorry but I really need to push back on this one. Earlier I did have a version of this that listed the files manually and then later one that was a little less manual but even then Jeroen complained that it made the file "too long" or something.

I also reject the characterization of this has Makefile "tricks". These are standard features of make that have been around forever. These objections just sound like you've never encountered a non-trivial makefile before (which I know isn't true).

Honestly I would just as soon get rid of this makefile entirely, but as long as it's there I see no problem here.

comment:29 follow-up: Changed 3 years ago by embray

In the meantime you're holding up real work by bikeshedding over code that you arbitrarily think is "too complicated".

comment:30 in reply to: ↑ 28 Changed 3 years ago by mkoeppe

Replying to embray:

I also reject the characterization of this has Makefile "tricks". These are standard features of make that have been around forever. These objections just sound like you've never encountered a non-trivial makefile before (which I know isn't true).

I've encountered too many.

comment:31 in reply to: ↑ 29 Changed 3 years ago by jdemeyer

Replying to embray:

In the meantime you're holding up real work by bikeshedding over code that you arbitrarily think is "too complicated".

I understand but it's not just bikeshedding. It's also about readability. Every time I look at that src/Makefile I really have to think hard to understand what is going on. And usually, that is a bad sign.

If you convince me that all that extra complexity is really needed, then I don't mind to merge this. But at this point, I am not yet convinced.

comment:32 Changed 3 years ago by embray

I find that unconvincing--have you ever looked at sage's toplevel Makefile? That's confusing. Given the other complexities of sage I do find nitpicking over the (highly subjective) "readability" of one makefile confusing. Maybe you're just not used to reading makefiles that happen to make use of these features, but I have seen plenty before I don't think it's unreadable at all.

If we're going to remove this makefile eventually anyways, which we should, then just let it be.

comment:33 follow-up: Changed 3 years ago by embray

And yes, it's needed, because the previous file was actually wrong. I already demonstrated this to you before weeks ago. If you're going to use make to require that generated files are up to date, then you have to actually provide the names of those generated files. Most of the time that can be provided implicitly by a pattern (e.g. %.c => %.o) but that's not the case here. In part because make was really not designed for stuff like this in the first place.

Last edited 3 years ago by embray (previous) (diff)

comment:34 Changed 3 years ago by embray

On further reflection, by my own logic if my preference is to remove the makefile entirely then it doesn't matter what's in there. So if it'll help get this done I'll remove the changes changes and focus on helping the efforts to improve Sage's build.

comment:35 Changed 3 years ago by jdemeyer

OK, maybe I already I asked you this, but why run a Python command to determine INTERP_SOURCES? Why not a simple wildcard?

Edit: never mind, this INTERP_SOURCES has generated sources.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:36 in reply to: ↑ 33 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

And yes, it's needed, because the previous file was actually wrong.

Maybe technically wrong but right for all practical purposes. Given that all these files are generated at the same time together, there is really nothing wrong with relying on the timestamp of just file.

comment:37 in reply to: ↑ 25 Changed 3 years ago by jdemeyer

Replying to embray:

The doctest runner still imports a module to determine if there are any doctests in the module.

Sorry to say this, but that can't be right. The Sage doctester looks at the files textually to determine the doctests. If what you say is true, we would have the same problem for the docbuilder (src/sage_setup/docbuild/__main__.py) but we don't...

For the sake of simplicity, I would remove that nodoctests block.

comment:38 follow-up: Changed 3 years ago by embray

However the doctester determines the doctests, the fact remains that at some point it imports the module. That happens here: https://git.sagemath.org/sage.git/tree/src/sage/doctest/sources.py#n670

The reason that isn't a problem for src/sage_setup/docbuild/__main__.py (and believe me, this had me pulling my hair out) is that it is listed in this insidious `in_lib` attribute. I described more problems with this in #20729.

But if you don't believe me try removing it yourself and see. Like I said, I wouldn't have even thought to add such a thing in the first place if I didn't have a good reason.

comment:39 in reply to: ↑ 36 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

And yes, it's needed, because the previous file was actually wrong.

Maybe technically wrong but right for all practical purposes. Given that all these files are generated at the same time together, there is really nothing wrong with relying on the timestamp of just file.

I encountered cases in practice where this was not reliable. But that was months ago now and I can't recall the specifics. Like I said, it doesn't matter. I don't care anymore since I'm going to work on getting rid of the makefile anyways.

comment:40 in reply to: ↑ 38 Changed 3 years ago by jdemeyer

Replying to embray:

However the doctester determines the doctests, the fact remains that at some point it imports the module. That happens here: https://git.sagemath.org/sage.git/tree/src/sage/doctest/sources.py#n670

The reason that isn't a problem for src/sage_setup/docbuild/__main__.py (and believe me, this had me pulling my hair out) is that it is listed in this insidious `in_lib` attribute. I described more problems with this in #20729.

OK, I commented on that ticket. Let me know what you think.

comment:41 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

comment:42 Changed 3 years ago by embray

Needs to be rebased again...

All the argument about the makefile is a moot point now that that makefile has been removed.

comment:43 Changed 3 years ago by jdemeyer

Also #20729 has been fixed in the mean time, so you shouldn't need to mess with the doctester anymore...

comment:44 Changed 3 years ago by embray

Right, thanks for the reminder. I'll work on this when I get back from vacation.

comment:45 Changed 3 years ago by git

  • Commit changed from 206138aaf56066e5cac608a792590e45b82c20f3 to 04c22e9aba7758da14974d5a3ba53f09349ab628

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bede9b3interpreters.py was quite long, so converted it to a package
320b3c4Changed indentation on several chunks of template code that was previously broken out of the normal Python indentation flow.
c346824Fixes a newline that messed up the formtting a bit.
9eb811fOne more whitespae fix
89b48beAdd some __main__ modules for ease of testing.
55853a7Just import everything into the top-level __init__ to rebuild the original sage_setup.autogen.interpreters namespace. This enures that all the tests pass.
9e5cf75Add from __future__ import print_function, absolute_import to all (non-empty) modules for forward compatibility.
45b2847Change the name attributes of the interpreter implementations to class
3530046Deduplicate code in anticipation of adding more generated file(s) for interpreters.
04c22e9More programmatic discovery of interpreter types rather than manually listing them

comment:46 Changed 3 years ago by embray

Rebased again, on top of current develop. That made this ticket much simpler, now that it doesn't require any build system changes.

There's not much to this now other than refactoring and code reformatting--no real substantive changes. I made sure to incorporate changes to the interpreters module that have been made since I last worked on this.

Last edited 3 years ago by embray (previous) (diff)

comment:47 Changed 3 years ago by tscrim

Back to needs review?

comment:48 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Yeah, meant to do that.

comment:49 Changed 3 years ago by embray

  • Milestone changed from sage-7.4 to sage-8.0

Would be nice to get this taken care of finally.

comment:50 Changed 3 years ago by jdemeyer

Conflicts with #20238 in various ways....

comment:51 Changed 3 years ago by git

  • Commit changed from 04c22e9aba7758da14974d5a3ba53f09349ab628 to b34d5d8fc509f6773fe3c72c3d1b0e24a358efde

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

a47bb08interpreters.py was quite long, so converted it to a package
4df82c8Changed indentation on several chunks of template code that was previously broken out of the normal Python indentation flow.
721cf50Fixes a newline that messed up the formtting a bit.
45ce2f1One more whitespae fix
6620390Add some __main__ modules for ease of testing.
241dc1bJust import everything into the top-level __init__ to rebuild the original sage_setup.autogen.interpreters namespace. This enures that all the tests pass.
eb5acf9Add from __future__ import print_function, absolute_import to all (non-empty) modules for forward compatibility.
1f46f30Change the name attributes of the interpreter implementations to class
f039d8cDeduplicate code in anticipation of adding more generated file(s) for interpreters.
b34d5d8More programmatic discovery of interpreter types rather than manually listing them

comment:52 Changed 3 years ago by embray

  • Dependencies set to #20238

Only minor conflicts. I've rebased on your branch in #20238.

comment:53 follow-up: Changed 2 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori

This looks like a good addition and will allow to close #21459 as a temporary but quick solution for Cygwin.

What about the dependency on #20238? Can we make the dependency the other way around? Or will #20238 be finished soon?

comment:54 in reply to: ↑ 53 Changed 2 years ago by jdemeyer

Replying to jpflori:

Or will #20238 be finished soon?

It is finished already since almost a month.

comment:55 Changed 2 years ago by embray

Now that you've given #20238 a positive review this should be fine as is.

comment:56 Changed 2 years ago by jpflori

  • Status changed from needs_review to positive_review

The split here is a very good idea. The autogenerating file was huge and a pain to parse for a human.

comment:57 Changed 2 years ago by vbraun

  • Branch changed from u/embray/interpreters-refactor to b34d5d8fc509f6773fe3c72c3d1b0e24a358efde
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.