sage.features.threejs: Fix modularization regression after #37024
#37178
Conversation
src/sage/features/threejs.py
Outdated
| try: | ||
| with open(filename) as f: | ||
| return f.read().strip() | ||
| except FileNotFoundError: | ||
| return "unknown" |
There was a problem hiding this comment.
This is overly complicated. Just kill the file and hardcode the string here. I'd even kill the whole "threejs version" business which is completely nuts and anti-modular. (Could all of this be replaced by https://github.com/jupyter-widgets/pythreejs ?)
Anyway, the whole modularization business is just too complicated. What do you mean "sage.features is shipped by sagemath-environment, but the version file that the Feature uses is shipped by sagemath-repl." Certainly all the pieces needed to use threejs (including the feature, etc) should go in the same "module".
Modularization should be about reducing complexity, not increasing it. Modules should have clear separation of concerns and communicate via clear well-defined interfaces.
There was a problem hiding this comment.
the whole modularization business is just too complicated.
Your sentence is not complete. When you say "too ...", you need to say "for ..." as well.
There was a problem hiding this comment.
the whole modularization business is just too complicated.
Your sentence is not complete. When you say "too ...", you need to say "for ..." as well.
No, I don't. I think it's too complicated, period.
There was a problem hiding this comment.
Modularization should be about reducing complexity, not increasing it.
You have already made it sufficiently clear that you're not interested, and that's fine; but I'll explain it nevertheless for the audience.
No, modularization is not primarily or absolutely about "reducing complexity". There are other criteria such as reusability and portability and deployability. Generally we decide to break out modules from a larger unit for a combination of such reasons. In the case of the Sage modularization project (#29705), "complexity" ranks last among these 4 criteria in importance.
There was a problem hiding this comment.
What do you mean "sage.features is shipped by sagemath-environment, but the version file that the Feature uses is shipped by sagemath-repl."
As you can see in
the Feature definitions are provided by the small distribution sagemath-environment, which by design has no dependencies -- it is for detecting, not providing features. That's the well-defined interface, separation of concerns etc.
sagemath-environment certainly should not ship the plotting support code that we have in sage/ext_data/threejs/ -- where the threejs-version.txt file resides.
There was a problem hiding this comment.
As for the registry itself: at first sight it looks like the kind of thing python entry points are designed for.
Yes, I did consider this briefly when I wrote the
all_featuresthing. But ultimately I think it's not the right tool for it.
Why not?
There was a problem hiding this comment.
I'm generally skeptical when using a "configuration file" is brought in as a proposed solution...
The actual code to check the feature presence and how to search for the feature in the system (when applicable) can still belong to the module.
That is already possible and does not require a redesign of the whole thing. See for example: https://github.com/sagemath/sage/blob/develop/src/sage/features/mip_backends.py#L33
There was a problem hiding this comment.
Given that your design allows the different packages/distributions to be split at arbitrary file boundaries, the file could still be
sage.features.threejs, but just shipped by a different distribution.
I thought I explained this already: If the module sage.features.threejs were shipped by a different distribution, then client code would have to already use try: ... except: when importing the feature.
There was a problem hiding this comment.
I thought I explained this already: If the module
sage.features.threejswere shipped by a different distribution, then client code would have to already usetry: ... except:when importing the feature.
- There's no "client code" for threejs outside of
sage.repl, so there's no harm shippingsage.features.threejstogether withsage.repl. - Even if there was client code, it could get the feature with something like
get_feature("threejs"), so thetry: import ...would be abstracted away.
It seems to me we are back to non-constructive mode.
There was a problem hiding this comment.
In the current implementation, sage.features.threejs gets the version info from a text file shipped as package data shipped by the responsible distribution. My fix just repairs this for the case that that distribution is not instsalled.
This could as well be replaced along lines similar to what you mention in https://github.com/sagemath/sage/pull/37178/files#discussion_r1470471049: try: except for importing a function that returns the version info.
Either one is fine with me. Reworking the whole of the design of sage.features does not seem interesting to me
|
Let's merge this simple fix please. |
Here your problem is that sagemath-repl fails to include threejs support files. AFAICT, threejs is a requirement for the ipython backends, as declared in their Your problem should go away when you add the required files to sagemath-repl. |
|
To correct what I stated earlier, the plotting support code is actually intended to be shipped with sagemath-plot, not sagemath-repl. See https://github.com/mkoeppe/sage/blob/t/32432/modularization_of_sagelib__break_out_a_separate_package_sagemath_polyhedra/pkgs/sagemath-plot/MANIFEST.in.m4 |
No, the problem is that, contrary to the design of |
Sure, but it's the ipython backend that claims to support threejs. If you ship the ipython backend with sagemath-repl then you have to ship the threejs support files with sagemath-repl. Or at least, make it so that the ipython backend realizes threejs support is not available and then not claim to support it... |
|
That's exactly what would be implemented by calling |
There is my mistake: there is no reason whatsoever that this needs to be a feature. |
No, it's not: |
|
I know that I haven't implemented what you just wished for! When I said "fixing that is what I'm doing here", "that" is calling to |
But that feature name does appear in a |
I added that. It's not necessary for tests to pass (given that threejs is "standard", not "optional"). Now, with your "fix" in this PR, the "threejs" feature will never be available when you test sagemath-repl. You just broke the feature. The only way the feature can work is if it knows the version, so either you ship the file with sagemath-environment, or you hardcode the version in the function. There: --- a/src/sage/ext_data/threejs/threejs-version.txt
+++ /dev/null
@@ -1 +0,0 @@
-r122
diff --git a/src/sage/features/threejs.py b/src/sage/features/threejs.py
index 4517523918d..c6b811dd36f 100644
--- a/src/sage/features/threejs.py
+++ b/src/sage/features/threejs.py
@@ -52,12 +52,7 @@ class Threejs(StaticFile):
sage: Threejs().required_version()
'r...'
"""
- from sage.env import SAGE_EXTCODE
-
- filename = os.path.join(SAGE_EXTCODE, 'threejs', 'threejs-version.txt')
-
- with open(filename) as f:
- return f.read().strip()
+ return "r122"
def all_features():It's a sin from a modularization point of view, but it's not worse than the alternatives. Beware: I think you'll break arch which ships a different version of threejs. But that's a design choice. |
You mean, when I test sagemath-repl in an environment where sagemath-plot is not installed. |
It's obviously strictly worse than my solution. |
I wish I wish with all my heart, to fly with dragons in a land apart.
No. Your distribution is broken because it's not shipping a file that is required to decide if threejs is present or not. At least as it is it errors out. Your commit introduces a silent bug where the feature will incorrectly claim threejs is not present when it is. This is not theoretical: since some doctests in sage.repl need threejs, and you want to ship this file in sage.plot, you are effectively disabling those tests. |
|
The problem in this discussion is that we're talking about something that is insufficiently defined (per docstring of the |
--- a/src/sage/features/threejs.py
+++ b/src/sage/features/threejs.py
@@ -6,7 +6,7 @@ from . import StaticFile
class Threejs(StaticFile):
r"""
A :class:`~sage.features.Feature` which describes the presence of
- threejs-sage in a few standard locations.
+ threejs-sage version r122 in a few standard locations.
EXAMPLES::
@@ -52,12 +52,7 @@ class Threejs(StaticFile):
sage: Threejs().required_version()
'r...'
"""
- from sage.env import SAGE_EXTCODE
-
- filename = os.path.join(SAGE_EXTCODE, 'threejs', 'threejs-version.txt')
-
- with open(filename) as f:
- return f.read().strip()
+ return "r122"
def all_features(): |
|
Now it's defined, but you've already explained yourself why that change is a bad idea -- this design is worse for distributions that want to ship a different version. |
The bad design starts with having the version in the file path, effectively creating a piece of software named |
|
It's a bit too easy to just call it "bad design". The versioning was introduced for a specific reason: It was claimed at that time that threejs does not make any compatibility guarantees between different versions (I have no first-hand knowledge of that). In the classic (pre-Jupyterlab) Jupyter notebook, extensions such as the threejs viewer would be installed in the frontend but would have to be compatible with multiple kernels (including different Sage installations). This concern may be outdated after the switch to the Jupyterlab technologies and its "federated" extensions. (I don't know the details of that.) Anyway, as a project we need to be able to merge a simple practical fix as in this PR without going on a meandering path of ever-widening scope. |
|
Marking as blocker - this needs to be resolved before the release. |
|
In spite of your long and bloated discussion, I want to suggest another solution. There is nothing wrong that from sage.env import SAGE_SHARE, THREEJS_DIR
threejs_search_path = THREEJS_DIR or (
os.path.join(SAGE_SHARE, "jupyter", "nbextensions", "threejs-sage"),
os.path.join(SAGE_SHARE, "sagemath", "threejs-sage"),
os.path.join(SAGE_SHARE, "sage", "threejs"),
os.path.join(SAGE_SHARE, "threejs-sage")
)
try:
version = self.required_version()
filename = os.path.join(version, "three.min.js")
except FileNotFoundError:
filename = 'unknown'
StaticFile.__init__(
self, name="threejs",
filename=filename,
spkg="threejs",
type="standard",
search_path=threejs_search_path,
description="JavaScript library to display 3D graphics") |
|
Thanks, I like this solution. |
|
With the fix, the test for sagemath-environment passes. Please add some comment to the |
|
Same dog with a different collar. The problem I raised is that For example, this breaks The simple solution is to add By design, the threejs feature depends on this file to do its job. Not shipping the version information together with the feature is a mistake and the |
No, you're mistaken about the design of
And the latter is what we're doing here. We are delegating the definition of what version is needed to the module that needs it. |
Source: I wrote the "threejs feature", and its documentation reads
Reasonable. It is a bug and has to be fixed.
Not reasonable. We should be very careful to ship features that give different answers depending on which combination of packages is installed.
You are still missing the fact that |
I would generally suggest more restraint with making declarations of this type.
Saying that
It does not have to be shipped with |
|
Let me ask some basic questions. What is the use of I find it strange that a feature test needs version information. I guess that no other feature test needs that. Can we do the threejs feature test without the version informaion? |
See #37178 (comment) above and #30123, which has all the links. In short, our venv contains a frontend (jupyter notebook), which is able to talk to multiple kernels installed in separate venvs. |
The package Where the current version of the package is The file Shipping |
Perhaps the tool can test presence of |
No, the feature is This feature is meant to inform whether the package threejs-sage is installed (in the system, in sage-the-distro, etc). Since sagemath is strict on the version required, we test for the exact version. There's no way to be correct about this test without knowing what is the required version. The version file is not part of the feature, is a support file for the feature.
Here's a trivial way to solve this: An alternative would be for the feature to test the presence of threejs-sage regardless of version. This is how it's done for everything else, e.g. singular feature tests for the presence of a |
Yes. I prefer this solution. |
@tornaria That's correct, but this does not imply at all that sage.features (sagemath-environment) needs to define the version itself. It uses delegation as already explained in #37178 (comment) |
|
I'm setting it back to "needs work". Based on the discussion, it is clear that I should improve the documentation of |
This makes the feature complete when only sagemath_environment is installed. Fixes: sagemath#37178
It's a bit confusing: the spkg includes a file named
Yes, sure.
Me too. |
|
I would also strongly prefer "1 version". But for this I would strongly suggest to look into using: ... and abandon our custom solution. I don't have time to work on that at the moment. |
|
It's okay to use delegation to discover if a feature is present, but what the feature is should be defined by the feature class itself (and hence by the distribution shipping the feature class). This becomes more apparent once you think about this on a more global level: for example, if another class (shipped in yet another distribution or perhaps in a user module) wants to check if the correct version of threejs is installed, then it should only depend on sage-environment. What if that other class needs a different version of threejs? So the solution of #37380 seems to make the most sense to me. As an alternative, it would make sense if features have a |
That's right, that's the correct level on which to think about it. But your conclusion is not correct. I'll elaborate when I get back to it, as I said in #37380 |
Another option I just came across: https://github.com/Mathics3/mathics-threejs-backend |
|
Fugly but probably a slight improvement, either way imho rather low priority. I propose to merge so you all can focus your energy on something more relevant |
|
Thanks. (But no, it's high priority, as marked.) |
sagemathgh-37178: `sage.features.threejs`: Fix modularization regression after sagemath#37024 <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> `sage.features` is shipped by **sagemath-environment**, but the version file that the Feature introduced in sagemath#37024 uses is shipped by **sagemath-repl**. This breaks doctesting in a modularized environment. As seen in ipython/ipython#14317 (https://github.com/ipytho n/ipython/actions/runs/7731330166/job/21078723213?pr=14317#step:9:136) <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37178 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
sagemathgh-37178: `sage.features.threejs`: Fix modularization regression after sagemath#37024 <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> `sage.features` is shipped by **sagemath-environment**, but the version file that the Feature introduced in sagemath#37024 uses is shipped by **sagemath-repl**. This breaks doctesting in a modularized environment. As seen in ipython/ipython#14317 (https://github.com/ipytho n/ipython/actions/runs/7731330166/job/21078723213?pr=14317#step:9:136) <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37178 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
|
Documentation preview for this PR (built with commit a2ce28c; changes) is ready! 🎉 |
sage.featuresis shipped by sagemath-environment, but the version file that the Feature introduced in #37024 uses is shipped by sagemath-repl.This breaks doctesting in a modularized environment. As seen in ipython/ipython#14317 (https://github.com/ipython/ipython/actions/runs/7731330166/job/21078723213?pr=14317#step:9:136)
📝 Checklist
⌛ Dependencies