Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove src/sage/__init__.py #34187

Closed
mkoeppe opened this issue Jul 15, 2022 · 39 comments
Closed

Remove src/sage/__init__.py #34187

mkoeppe opened this issue Jul 15, 2022 · 39 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jul 15, 2022

Follow-up from #33011.

Because of the complication described in #33011, we not only remove the file from the source tree but also remove it from site-packages before building sagelib.

Follow-up:

  • make sagemath-standard, sage-setup depend on sagemath-environment

Depends on #33011

CC: @kwankyu @videlec

Component: refactoring

Author: Matthias Koeppe

Branch: a77b971

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/34187

@mkoeppe mkoeppe added this to the sage-9.7 milestone Jul 15, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

Dependencies: #33011

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

comment:2

If I remove __init__.py from src/sage and pkgs/sagemath-standard/build/lib.macosx-12-x86_64-cpython-39/sage, then sage is built without problem, but doctest fails. Is this expected?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

Commit: f447a11

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

comment:4

I'm getting

sage -t --random-seed=29572225487268684535188462271558972615 src/sage/misc/sageinspect.py  # 1 doctest failed
sage -t --random-seed=29572225487268684535188462271558972615 src/sage_setup/find.py  # 13 doctests failed
sage -t --random-seed=29572225487268684535188462271558972615 src/sage_setup/clean.py  # 4 doctests failed
sage -t --random-seed=29572225487268684535188462271558972615 src/sage/misc/edit_module.py  # 1 doctest failed

Last 10 new commits:

1326e17src/sage/numerical/__init__.py: Remove
5aebcf3src/sage_docbuild/builders.py: Handle namespace packages
bba5284src/sage_setup/find.py: Update doctest output
56e9123Merge #28925
6a76779Merge #28925
bd185b5Merge tag '9.7.beta5' into t/33011/remove___init___py_files_for_packages_designated_to_be_namespace_packages
39aa2f1src/sage_setup/command/sage_install.py (sage_clean): Clean __init__.py when installed package was ordinary, source package is namespace
d3bdb46Merge #33011
7336e9dbuild/pkgs/sagelib/spkg-install: Remove installed sage/__init__.py
f447a11src/sage/__init__.py: Remove

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

comment:5
> /Users/mkoeppe/s/sage/sage-rebasing/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage_setup/find.py(278)_cythonized_dir()
    276         src_dir = Path(src_dir)
    277         sage = import_module('sage')
--> 278         d = Path(sage.__file__).resolve().parent.parent
    279         editable_install = d == src_dir.resolve()
    280     if editable_install:

this use of sage.__file__ now needs to be changed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

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

01f9143src/sage_setup: Fix for namespace package sage

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

Changed commit from f447a11 to 01f9143

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

Changed commit from 01f9143 to 9627219

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

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

9627219src/sage/misc/edit_module.py: Update doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

Changed commit from 9627219 to e884c9e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2022

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

e884c9esrc/sage/misc/sageinspect.py: Update doctest

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 15, 2022

Author: Matthias Koeppe

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 15, 2022

comment:10

Perhaps we have to respect the deprecation period of isfunction() defined in sage and deprecated by #32479. It ends after two months from now.

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Jul 16, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 16, 2022

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

7a0ff3fsrc/sage/all.py: Restore sage.isfunction as a LazyImport

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 16, 2022

Changed commit from e884c9e to 7a0ff3f

@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.7 Jul 16, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Jul 18, 2022

comment:14

Sorry for pointing to a minor thing, but the deprecation message is somewhat misleading as the function in sage.misc.sageinspect is is_function_or_cython_function.

sage: isfunction(f)
<ipython-input-3-631f9d828a9a>:1: DeprecationWarning: 
Importing isfunction from here is deprecated. If you need to use it, please import
 it directly from sage.misc.sageinspect
See https://trac.sagemath.org/32479 for details.
  isfunction(f)
True
sage: from sage.misc.sageinspect import isfunction
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-4-53c0fed64cdc> in <module>
----> 1 from sage.misc.sageinspect import isfunction

ImportError: cannot import name 'isfunction' from 'sage.misc.sageinspect' (/Users/kwankyu/GitHub/sage-temp/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/misc/sageinspect.py)

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 18, 2022

comment:15

Perhaps moving isfunction in sage.__init__.py to sage.all is a solution?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

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

dc2ddb5src/sage/misc/lazy_import.pyx: Improve deprecation message when as_ is in use

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

Changed commit from 7a0ff3f to dc2ddb5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

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

adb89ffsrc/sage/misc/lazy_import.pyx: Improve deprecation message when as_ is in use

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2022

Changed commit from dc2ddb5 to adb89ff

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 18, 2022

comment:18

I've implemented a more general solution:

sage: from sage import isfunction
sage: isfunction
/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/sage/repl/display/fancy_repr.py:272: DeprecationWarning: 
Importing isfunction from here is deprecated; please use "from sage.misc.sageinspect import is_function_or_cython_function as isfunction" instead.
See https://trac.sagemath.org/32479 for details.
  output = repr(obj)
<function is_function_or_cython_function at 0x10f957ee0>

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 18, 2022

comment:19

That is a right solution. Thanks.

As a side remark, it seems to me that the lazy_import with deprecation capability is misnamed since "lazy import" itself has nothing to do with deprecation. Perhaps LazyImport object can just have a hook to a function, which is called when import happens, and we may have deprecated_import that issues the deprecation warning when import happens. Thus we have deprecated_import separate from the plain lazy_import.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 18, 2022

comment:20

I don't know, I think it's just an extra feature of lazy_import that is activated by a keyword. I don't see anything wrong with that

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2022

comment:21

Replying to @mkoeppe:

I don't know, I think it's just an extra feature of lazy_import that is activated by a keyword. I don't see anything wrong with that.

a lazy_import with deprecation keyword is to be removed after the deprecation period while a plain lazy_import is to remain. What seems wrong to me is that this distinction is implicitly made by the mere presence of a keyword argument.

Anyway, this ticket looks good to me.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2022

comment:22

doctest failures due to the changed deprecation message:

sage -t src/sage/functions/other.py  # 1 doctest failed
sage -t src/sage/tests/books/computational-mathematics-with-sagemath/graphique_doctest.py  # 1 doctest failed
sage -t src/sage/modular/modform/find_generators.py  # 2 doctests failed
sage -t src/sage/groups/matrix_gps/homset.py  # 1 doctest failed
sage -t src/sage/schemes/hyperelliptic_curves/all.py  # 4 doctests failed
sage -t src/sage/rings/all.py  # 3 doctests failed

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2022

comment:23

Ah, I forgot about that

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2022

Changed commit from adb89ff to a77b971

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2022

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

a77b971Update doctest output for changed lazy_import deprecation warnings

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2022

Reviewer: Kwankyu Lee

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 19, 2022

comment:25

LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 19, 2022

comment:26

Thank you!

@vbraun
Copy link
Member

vbraun commented Aug 1, 2022

Changed branch from u/mkoeppe/remove_src_sage___init___py to a77b971

@saraedum
Copy link
Member

saraedum commented Oct 7, 2022

Changed commit from a77b971 to none

@saraedum
Copy link
Member

saraedum commented Oct 7, 2022

comment:28

When building surface-dynamics (a Cython project that builds against sagelib) we are getting

  surface_dynamics/flat_surfaces/origamis/origami_dense.pyx:39:8: 'sage/structure/element.pxd' not found

It seems to me that Cython does not find the .pxd file anymore because the __init__.py in the base directory is missing now.

@saraedum
Copy link
Member

saraedum commented Oct 7, 2022

comment:29

This seems to be cython/cython#2918 essentially.

@saraedum
Copy link
Member

saraedum commented Oct 7, 2022

comment:30

We could patch the cython packaged in conda-forge with cython/cython#4918 but maybe there is some way to fix this that works for unpatched Cythons?

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 7, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants