using ``installed`` property to skip tests by mmatera · Pull Request #362 · Mathics3/mathics-core

mmatera · 2022-06-10T14:16:15Z

This PR is a previous step for adding a Pyston test workflow (see #368). The proposed changes allow skipping the tests associated with symbols whose evaluation requires not available external libraries.

To check that indeed this works with a minimal environment, I added here a workflow to check that in standard Ubuntu Python.

rocky · 2022-06-10T18:16:56Z

mathics/doc/common_doc.py

+
+
+def check_requires_list(requires: list) -> bool:
+    global requires_cache


This line does nothing.

I put this for clarity, but OK.

Huh? There is no variable requires_cache. The typical way of adding clarity is in a docstring. For example:

""" Check if module names in ``requires`` can be imported and return True if they can or False if not. This state value is also recorded in dictionary `requires_lib_cache` keyed by module name and is used to determine whether to skip trying to get information from the module. """

You are right! I changed the name. The variable was requires_lib_cache, which was defined just above.

Since mathics.builtin.base also benefits from using check_requires_list I moved the code there.

rocky · 2022-06-10T18:21:10Z

mathics/doc/common_doc.py

                == self.pymathicsmodule.__name__
            ):  # nopep8
+                if not check_requires_list(var):
+                    print("  skiping ", var.__name__)


skiping -> skipping. But I'd be more explicit to why it is skipped, an optional package is not installed.

This print actually never happens, because we are not generating documentation for pymathics modules. I just removed it.

rocky · 2022-06-10T22:11:55Z

Overall this looks good and is much needed. I'd like to try it out though and haven't had a chance.

mmatera · 2022-06-10T22:20:14Z

mathics/doc/common_doc.py

+    Check if module names in ``requires`` can be imported and return True if they can or False if not.
+
+    This state value is also recorded in dictionary `requires_lib_cache` keyed by module name and is used to determine whether to skip trying to get information from the module."""
+    global requires_lib_cache


This is what I had in mind. Having or not this line does not seems to change anything, just to state that require_lib_cache is a variable at the level of the module.

Please don't add a global. It is frowned upon generally. Some would argue that this should be passed as a parameter, and that is the typical way you convey this - in the interface.

I know this kind of thing, a creative use of global seems innovative and clever. Along those lines, I used to (and still sometimes do) add pass statements where in most programming languages an end closer would appear. However it puts off most programmers so I generally don't do it except in code where I am pretty much the only one who will ever use this.

There is a place for creativity, but here and this way sadly isn't the place for that.

It was not about creativity, but being more verbose about what I am changing here. I think the new place of this stuff in mathics.builtin.basealso goes in this direction.

Regarding this, probably the cache for the previous results is not needed: Python have also a cache inside importlib, so the global variable does not provide too much advantage.

mmatera · 2022-06-10T22:26:28Z

Overall this looks good and is much needed. I'd like to try it out though and haven't had a chance.

Actually, there are several things to improve in this module, but I refrain to do it in order to make this PR as simple as possible to get something that works with the basic Pyston test workflow.

rocky · 2022-06-10T22:32:08Z

Overall this looks good and is much needed. I'd like to try it out though and haven't had a chance.

Actually, there are several things to improve in this module, but I refrain to do it in order to make this PR as simple as possible to get something that works with the basic Pyston test workflow.

Sure. Yes, it is useful when working with Pyston which I think will be doing more of in the futre.

I just want to double check and try it so there isn't something kind of silly like there ~~was~~ is in the --time-each option of docpipeline.py when that slipped in.

rocky · 2022-06-11T13:33:37Z

Actually, there are several things to improve in this module,

Please record those items as issues. Thanks.

I just tried without scikit_image using pyston-2.3.3. I get:

python mathics/docpipeline.py -x
Testing Mathics 4.1.0.dev0
...
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.

mmatera · 2022-06-11T13:42:28Z

Actually, there are several things to improve in this module,

Please record those items as issues. Thanks.

I just tried without scikit_image using pyston-2.3.3. I get:

python mathics/docpipeline.py -x
Testing Mathics 4.1.0.dev0
...
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.

this is in this branch?

rocky · 2022-06-11T13:48:17Z

Actually, there are several things to improve in this module,

Please record those items as issues. Thanks.
I just tried without scikit_image using pyston-2.3.3. I get:

python mathics/docpipeline.py -x
Testing Mathics 4.1.0.dev0
...
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.

this is in this branch?

I believe so:

$ git status
On branch improve_install_check_in_docs
Your branch is up to date with 'origin/improve_install_check_in_docs'.
git log | head
commit 4fde248a5dd2e88eeb3a50707f9789c646751061
Author: mmatera <matera@fisica.unlp.edu.ar>
Date:   Fri Jun 10 19:18:33 2022 -0300

    adding docstring. fixing variable name

commit 53a52465e1f6ee1254a5e916daf90aa8f5821906
Author: mmatera <matera@fisica.unlp.edu.ar>
Date:   Fri Jun 10 19:05:06 2022 -0300

mmatera · 2022-06-13T19:33:34Z

Actually, there are several things to improve in this module,

Please record those items as issues. Thanks.
I just tried without scikit_image using pyston-2.3.3. I get:

python mathics/docpipeline.py -x
Testing Mathics 4.1.0.dev0
...
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.
Test failed: BinaryImageQ in Reference of Built-in Symbols / Graphics, Drawing, and Images
Reference of Built-in Symbols
Result: False
Wanted: True
Additional output:
General::pyimport: Binarize[] is not available. Python module "skimage" is not installed.

this is in this branch?

Now I added a workflow testing against a basic installation of Python 3.9 in ubuntu. #368 should show that this also works in Pyston.

rocky · 2022-06-13T23:03:44Z

. #368 should show that this also works in Pyston.

That is a draft and this isn't. That may show that there is a particular way to install Pyston that works. However I've been running Pyston with scikit-image installed for a while even without this PR.

What i am wondering about though is whether this addresses the problem I encountered. Not whether there is some scenario where we can make pyston work.

It would help if you can describe a specific setup and test case where things were failing without this patch and then with this they don't fail.

mmatera · 2022-06-13T23:30:55Z

. #368 should show that this also works in Pyston.

That is a draft and this isn't. That may show that there is a particular way to install Pyston that works. However I've been running Pyston with scikit-image installed for a while even without this PR.

No: the idea is not to show that Pyston can run with scikit-image, but that can run without it.

What i am wondering about though is whether this addresses the problem I encountered. Not whether there is some scenario where we can make pyston work.

It would help if you can describe a specific setup and test case where things were failing without this patch and then with this they don't fail.

With master, if scikit image is not installed, there are tests that fail (as you show above). However, we say that scikit is an optional package, so the CI should show in green if the environment does not have scikit available. The same happens with scipy. This PR fixes that and shows that it works by adding a workflow that has a minimal installation.

mmatera · 2022-06-13T23:38:58Z

The problem with having a workflow with the full set of libraries in Pyston is that most of the time is invested in recompiling scipy, scikit and other heavy libraries. When that libraries are available, then the tests that assume that the libraries are there run.

I am looking for possibilities for downloading a preinstalled environment with all the optional libraries, but still I have not give with a solution. On the other hand, if we are looking to do performance tests in the evaluation process, I think that also makes sense to check it over a minimal environment.

mmatera · 2022-06-14T22:24:23Z

@rocky, in #370 I tried a workflow for Pyston with the full set of optional libraries. It took ~60 min, against ~6 min with the reduced set (#368). 48 minutes were used to compile the libraries.

. #368 should show that this also works in Pyston.

That is a draft and this isn't. That may show that there is a particular way to install Pyston that works. However I've been running Pyston with scikit-image installed for a while even without this PR.

What i am wondering about though is whether this addresses the problem I encountered. Not whether there is some scenario where we can make pyston work.

It would help if you can describe a specific setup and test case where things were failing without this patch and then with this they don't fail.

@rocky, in #370 I tried a workflow for Pyston with the full set of optional libraries. It took ~60 min, against ~6 min with the reduced set (#368). 48 minutes were used to compile the libraries. So, if we are trying to check that the mathics core runs properly in Pyston, I think we should try it against the basic installation. There is where this PR is needed: if you just add the workflow to master, several tests are not passed (like the ones in the mathics.builtin.drawing.image module).

mmatera · 2022-06-18T15:14:35Z

.github/workflows/ubuntu-minimal.yml

@@ -0,0 +1,34 @@
+name: Mathics (ubuntu-minimal)


This workflow was added to check that indeed with the changes the tests pass with a minimal installation.

mmatera · 2022-06-20T20:08:05Z

Superseeded.

rocky reviewed Jun 10, 2022

View reviewed changes

mmatera commented Jun 10, 2022

View reviewed changes

rebase

2ed335b

mmatera force-pushed the improve_install_check_in_docs branch from 4fde248 to 2ed335b Compare June 13, 2022 16:54

missing import

9565593

fix the reversion in calculus.py

0fe18ac

mmatera mentioned this pull request Jun 18, 2022

Pyston workflow #368

Closed

mmatera commented Jun 18, 2022

View reviewed changes

mmatera closed this Jun 20, 2022



		def check_requires_list(requires: list) -> bool:
		global requires_cache

Uh oh!

Conversation

mmatera commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky commented Jun 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rocky Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatera commented Jun 10, 2022

Uh oh!

rocky commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rocky commented Jun 11, 2022

Uh oh!

mmatera commented Jun 11, 2022

Uh oh!

rocky commented Jun 11, 2022

Uh oh!

mmatera commented Jun 13, 2022

Uh oh!

rocky commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmatera commented Jun 13, 2022

Uh oh!

mmatera commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmatera commented Jun 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatera commented Jun 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mmatera commented Jun 10, 2022 •

edited

Loading

rocky Jun 10, 2022 •

edited

Loading

rocky Jun 10, 2022 •

edited

Loading

rocky commented Jun 10, 2022 •

edited

Loading

rocky commented Jun 13, 2022 •

edited

Loading

mmatera commented Jun 13, 2022 •

edited

Loading