Fix fricas doctest pickling #40570

cxzhong · 2025-08-11T10:06:30Z

Fix FriCAS doctest pickling failures

Problem: Doctests in sage.interfaces.fricas were failing with TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object when testing fricas == loads(dumps(fricas)).

sage: fricas == loads(dumps(fricas))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
File ~/sage/src/sage/misc/persist.pyx:338, in sage.misc.persist.dumps()
    337         type_obj = type((<LazyImport>obj).get_object())
--> 338     ans = type_obj.dumps(obj, compress)
    339 except (AttributeError, RuntimeError, TypeError):

File ~/sage/src/sage/structure/sage_object.pyx:508, in sage.structure.sage_object.SageObject.dumps()
    507
--> 508         return _base_dumps(self, compress=compress)
    509

File ~/sage/src/sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps()
    305 global already_pickled
--> 306 gherkin = SagePickler.dumps(obj)
    307 already_pickled = {}

File ~/sage/src/sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps()
    829 pickler = cls(buf, **kwargs)
--> 830 pickler.dump(obj)
    831 already_pickled = {}

TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object

During handling of the above exception, another exception occurred:

TypeError                                 Traceback (most recent call last)
Cell In[1], line 1
----> 1 fricas == loads(dumps(fricas))

File ~/sage/src/sage/misc/persist.pyx:340, in sage.misc.persist.dumps()
    338     ans = type_obj.dumps(obj, compress)
    339 except (AttributeError, RuntimeError, TypeError):
--> 340     ans = _base_dumps(obj, compress=compress)
    341 already_pickled = {}
    342 return ans

File ~/sage/src/sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps()
    304
    305     global already_pickled
--> 306     gherkin = SagePickler.dumps(obj)
    307     already_pickled = {}
    308

File ~/sage/src/sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps()
    828 buf = io.BytesIO()
    829 pickler = cls(buf, **kwargs)
--> 830 pickler.dump(obj)
    831 already_pickled = {}
    832 return buf.getvalue()

TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object

Solution: Replace the problematic equality test with isinstance(loads(dumps(a)), FriCAS) using a fresh FriCAS instance instead of the global fricas object, which contains unpicklable lazy imports.

Changes:

Modified doctests in FriCAS.__init__ and FriCAS._quit_string methods
Preserves serialization testing while avoiding LazyImport pickling issues

📝 Checklist

The title is concise and informative.
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 and checked the documentation preview.

⌛ Dependencies

Replace problematic doctest 'fricas == loads(dumps(fricas))' with 'isinstance(loads(dumps(a)), FriCAS)' where a is a fresh FriCAS instance. The original test was failing because the global 'fricas' object contains module-level lazy imports that cannot be pickled. Using a fresh instance and checking type instead of equality avoids the LazyImport pickling issue while still validating serialization round-trip functionality. Fixes doctest failures in sage.interfaces.fricas.FriCAS.__init__ and sage.interfaces.fricas.FriCAS._quit_string methods.

…ests

Fix ruff linting error W293 by removing trailing whitespace from blank line in doctest.

Remove malformed literal block structure in TESTS section that was causing RST218 error. Properly format the doctest examples.

Remove TESTS:: header that was causing RST218 literal block formatting error while preserving the meaningful doctest that verifies new process starts after quit(). This addresses linting compliance while maintaining documentation quality.

github-actions · 2025-08-11T11:22:47Z

Documentation preview for this PR (built with commit e44838f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

tscrim · 2025-08-13T01:55:37Z

This test works for me (both from the command line and running sage -t on the interface file). The test should resolve the lazy import, which would make it pass. You have fricas installed, correct?

cxzhong · 2025-08-13T05:04:14Z

This test works for me (both from the command line and running sage -t on the interface file). The test should resolve the lazy import, which would make it pass. You have fricas installed, correct?

@tscrim do you use system's fricas or sage's fricas？Thank you for your reply.

tscrim · 2025-08-13T06:01:40Z

This test works for me (both from the command line and running sage -t on the interface file). The test should resolve the lazy import, which would make it pass. You have fricas installed, correct?

@tscrim do you use system's fricas or sage's fricas？Thank you for your reply.

@cxzhong Sage's fricas.

cxzhong · 2025-08-13T07:47:13Z

This test works for me (both from the command line and running sage -t on the interface file). The test should resolve the lazy import, which would make it pass. You have fricas installed, correct?

@tscrim do you use system's fricas or sage's fricas？Thank you for your reply.

@cxzhong Sage's fracas.

@tscrim Thank you. I rebuild sage and fricas. But I found that

sage: fricas == loads(dumps(fricas))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
File sage/misc/persist.pyx:338, in sage.misc.persist.dumps()

File sage/structure/sage_object.pyx:508, in sage.structure.sage_object.SageObject.dumps()

File sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps()

File sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps()

TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object

During handling of the above exception, another exception occurred:

TypeError                                 Traceback (most recent call last)
Cell In[1], line 1
----> 1 fricas == loads(dumps(fricas))

File sage/misc/persist.pyx:340, in sage.misc.persist.dumps()

File sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps()

File sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps()

TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object
sage: fricas(3)
3
sage: fricas == loads(dumps(fricas))
True

I run fricas(3) then run fricas == loads(dumps(fricas)), it will return the right result. But directly run fricas == loads(dumps(fricas)) then it leads to error. and I do not know how to deal with it.
OS: Ubuntu 25.04 WSL
Python: 3.13.5
Sage: Sage 10.7 with the sage's fricas

tscrim · 2025-08-18T08:44:34Z

Sorry for a delayed response. Does this mean you figured out the problem was local to your system? Otherwise why was this closed?

cxzhong · 2025-08-18T08:50:27Z

Sorry for a delayed response. Does this mean you figured out the problem was local to your system? Otherwise why was this closed?

I do not know how to solve it. Maybe It is just a problem with my system. So, I just closed it. Thank you for your reply. @tscrim

cxzhong · 2025-10-20T06:46:07Z

@nbruin Then I reopen this. I think it is pickle a real object.

cxzhong · 2025-10-20T06:49:24Z

@orlitzky

nbruin · 2025-10-20T07:20:38Z

I think the replacement test makes a lot of sense. You really want to test the pickling behaviour of fricas here; not that of a lazily imported object.

user202729 · 2025-10-20T11:00:06Z

I think the replacement test makes a lot of sense. You really want to test the pickling behaviour of fricas here; not that of a lazily imported object.

wait… why do we care about picklability of the interface object anyway? I feel x == loads(dumps(x)) is just the "placeholder" test when there isn't anything worth testing. (I see many other instances of that elsewhere.)

if the intention is to test that fricas is usable from the top-level user namespace then the other doctests already test that.

(but whatever, it isn't doing any harm)

another note.

sage.all_cmdline contains this

for pkg in ['axiom', 'fricas', 'gap', 'gap3', 'giac', 'gp',
            'gnuplot', 'kash', 'magma', 'macaulay2', 'maple',
            'mathematica', 'mathics', 'matlab',
            'mupad', 'mwrank', 'octave', 'qepcad', 'singular',
            'sage0', 'lie', 'r']:
    lazy_import(f'sage.interfaces.{pkg}', f'{pkg}_console')
del pkg

does these other systems have the same issue? (evidently yes. Except that they don't have load/dump test. Then why having one here?)

that test has been there since the very beginning. Might as well be historical accident (or that it actually work before).

#34547 adds this part (thus break the test, probably).

orlitzky · 2025-10-20T13:09:21Z

I don't know where to find it these days, but we used to have a coverage report that would complain about untested methods. This was usually a very good thing but in corner cases it would encourage you to write a dumb test for something that didn't really need its own test.

user202729 · 2025-10-20T13:43:04Z

I don't know where to find it these days, but we used to have a coverage report that would complain about untested methods. This was usually a very good thing but in corner cases it would encourage you to write a dumb test for something that didn't really need its own test.

there's still the coverage-report workflow running. The status report as GitHub annotation appears to stop working some time ago for some reason (possibly some major overhaul like the meson migration or the CI refactor), and nobody actually looked.

looking at one of the recent pull request e.g. #41082 you can still click through https://github.com/sagemath/sage/actions/runs/18637280279/job/53131673368?pr=41082#step:13:314 and reach https://app.codecov.io/github/sagemath/sage/commit/f29eb1ade0cd78d4d125699c40f603496fbdfc18 . I'm not entirely sure how to read that, but looks like the information is there.

for comparison, the GitHub annotation used to look like this. https://github.com/sagemath/sage/pull/38259/files#diff-2c12e62924e49ca61ef92a008aa860551fa8a4c6ad841061eab030b24d28b1e4R869

nbruin · 2025-10-20T15:01:45Z

It looks like that coverage test is actually keeping track of which lines get executed (and hence can find untested branches in the code. It used to be more primitive: just keeping track of whether each function ended up being called during testing. A test like A==loads(dumps(A)) is actually quite a strict test and it therefore became a bit of a standard test to see if serialization worked.

I don't know how it would work for an interface object: make sure that the "global" interface object is just looked up and returned? The serialization in this case would probably come into play for message-passing parallel things, where an object must be serialized and sent to another process. That's always going to be fairly fragile with objects in an interface (although sage does try to have those relatively state-free).

In any case, when fricas is actually a lazy import object, you're definitely not testing the right thing, so the test now is better, I think. fricas is optional and, as far as I know, hasn't been used extensively from sage. I would not expect its interface to be very sophisticated for that reason. So I'm not too surprised about running into basic test failures.

tscrim · 2025-10-21T20:11:33Z

Note that any lazily imported object should be resolved and replaced by the first call. So the change in the test should be fully equivalent to the current test. Any such failure is a bug in the testing framework with lazily imported objects.

tscrim · 2025-10-21T20:14:37Z

I don't know where to find it these days, but we used to have a coverage report that would complain about untested methods.

Is it part of the lint check? It was nice to have that… At least we can call it manually by sage —coverage X.

This was usually a very good thing but in corner cases it would encourage you to write a dumb test for something that didn't really need its own test.

Those “dumb” tests can be much more useful than you are giving them credit for. Also, there should be absolutely no exception to the 100% doctest (for new/modified functions) coverage rules.

nbruin · 2025-10-21T21:28:35Z

Note that any lazily imported object should be resolved and replaced by the first call. So the change in the test should be fully equivalent to the current test. Any such failure is a bug in the testing framework with lazily imported objects.

That was the attempted fix on #41054 . It's tricky, though. When you ask fricas == loads(dumps(fricas)), where fricas is a lazy import object, the LHS is already determined to be the lazy object (there's no call involved in retrieving that). Subsequently, dumps(fricas) would have a chance of importing and rebinding in the namespace, but the LHS has been determined by then already. So by then we're reliant on the __eq__ on the lazy import object being transparent.

The fix in #41054 was lowered in priority mainly due to the observation that https://peps.python.org/pep-0810/ would perhaps get native lazy imports into python. We'd eventually migrate to that.

It's also strange to pickle lazy imports: if it's worth pickling, why wasn't it loaded already? It seems to me almost always an indication of something fishy if code ends up trying to pickle a lazy import object.

tscrim · 2025-10-21T22:02:19Z

Note that any lazily imported object should be resolved and replaced by the first call. So the change in the test should be fully equivalent to the current test. Any such failure is a bug in the testing framework with lazily imported objects.

That was the attempted fix on #41054 . It's tricky, though. When you ask fricas == loads(dumps(fricas)), where fricas is a lazy import object, the LHS is already determined to be the lazy object (there's no call involved in retrieving that). Subsequently, dumps(fricas) would have a chance of importing and rebinding in the namespace, but the LHS has been determined by then already. So by then we're reliant on the __eq__ on the lazy import object being transparent.

There is no way to determine if it is a lazy import. If we do something like type, then the object gets resolved. Similarly, the __eq__ call should resolve the import. Why is it any different than fricas == 2 or fricas == fricas?

The fix in #41054 was lowered in priority mainly due to the observation that https://peps.python.org/pep-0810/ would perhaps get native lazy imports into python. We'd eventually migrate to that.

Indeed, it would be great if that got in. Although it would have to wait 5-10 years after it incorporated (assuming it does) in order to not force people to upgrade their Python to such a current version...

It's also strange to pickle lazy imports: if it's worth pickling, why wasn't it loaded already? It seems to me almost always an indication of something fishy if code ends up trying to pickle a lazy import object.

This might happen when doing a computation in parallel, where communication is done via pickling. For an interface, it shouldn't be shared, but it might be passing some lazily imported algebraic object (e.g., NN, which has been known to have problems with being resolved, but that's a separate issue).

I don't experience any issues with the test as written though. However, we could just explicitly resolve the import in a line beforehand as a hackaround...

nbruin · 2025-10-21T23:00:27Z

There is no way to determine if it is a lazy import. If we do something like type, then the object gets resolved. Similarly, the __eq__ call should resolve the import. Why is it any different than fricas == 2 or fricas == fricas?

Not true (and in fact sage.misc.lazy_import.clean_namespace relies on this):

sage: lazy_import('sage.rings.integer_ring','ZZ',as_='L')
sage: type(L)
<class 'sage.misc.lazy_import.LazyImport'>
sage: L
Integer Ring
sage: type(L)
<class 'sage.rings.integer_ring.IntegerRing_class'>

there are things that can happen with objects without a method being called on them. And there are some methods that are present on lazy_import objects that get fetched without going through __getattribute__, so they don't automatically lead to resolution of the object. As was found, __reduce__ and __reduce_ex__ are among those. I think __eq__ is properly shadowed, so fricas == loads(dumps(fricas)) could work with a lazy_import object. You're welcome to revive #41054 if you want. I think there's merit to it. Just not to a fricas doctest verifying the lazy_import behaviour.

user202729 · 2025-10-21T23:35:04Z

There is no way to determine if it is a lazy import.

as far as I can tell, type() is "very magical" and not overridable. Thus there is always a way to determine if something is a lazy import, call type() on it.

(https://stackoverflow.com/questions/56879245/)

That said, make dumps(<lazy import object>) returns dumps(<the target object>) sounds reasonable to me—assume it is implementable.

sagemathgh-40570: Fix fricas doctest pickling       Fix FriCAS doctest pickling failures Problem: Doctests in sage.interfaces.fricas were failing with TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object when testing ```fricas == loads(dumps(fricas))```. ``` sage: fricas == loads(dumps(fricas)) ------------------------------------------------------------------------ --- TypeError Traceback (most recent call last) File ~/sage/src/sage/misc/persist.pyx:338, in sage.misc.persist.dumps() 337 type_obj = type((<LazyImport>obj).get_object()) --> 338 ans = type_obj.dumps(obj, compress) 339 except (AttributeError, RuntimeError, TypeError): File ~/sage/src/sage/structure/sage_object.pyx:508, in sage.structure.sage_object.SageObject.dumps() 507 --> 508 return _base_dumps(self, compress=compress) 509 File ~/sage/src/sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps() 305 global already_pickled --> 306 gherkin = SagePickler.dumps(obj) 307 already_pickled = {} File ~/sage/src/sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps() 829 pickler = cls(buf, **kwargs) --> 830 pickler.dump(obj) 831 already_pickled = {} TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object During handling of the above exception, another exception occurred: TypeError Traceback (most recent call last) Cell In[1], line 1 ----> 1 fricas == loads(dumps(fricas)) File ~/sage/src/sage/misc/persist.pyx:340, in sage.misc.persist.dumps() 338 ans = type_obj.dumps(obj, compress) 339 except (AttributeError, RuntimeError, TypeError): --> 340 ans = _base_dumps(obj, compress=compress) 341 already_pickled = {} 342 return ans File ~/sage/src/sage/misc/persist.pyx:306, in sage.misc.persist._base_dumps() 304 305 global already_pickled --> 306 gherkin = SagePickler.dumps(obj) 307 already_pickled = {} 308 File ~/sage/src/sage/misc/persist.pyx:830, in sage.misc.persist.SagePickler.dumps() 828 buf = io.BytesIO() 829 pickler = cls(buf, **kwargs) --> 830 pickler.dump(obj) 831 already_pickled = {} 832 return buf.getvalue() TypeError: cannot pickle 'sage.misc.lazy_import.LazyImport' object ``` Solution: Replace the problematic equality test with isinstance(loads(dumps(a)), FriCAS) using a fresh FriCAS instance instead of the global fricas object, which contains unpicklable lazy imports. Changes: Modified doctests in ```FriCAS.__init__ ``` and ```FriCAS._quit_string``` methods Preserves serialization testing while avoiding LazyImport pickling issues ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#40570 Reported by: Chenxin Zhong Reviewer(s):

cxzhong added 7 commits August 11, 2025 17:50

Ensure a new process is started before quit() in FriCAS interface t…

4ef41f0

…ests

Ensure a new process is started BEFORE quit() in FriCAS interface t…

b12af57

…ests

fix a typo

16b6d19

Fix W293: Remove whitespace from blank line

b8f6597

Fix ruff linting error W293 by removing trailing whitespace from blank line in doctest.

Fix RST218: Fix docstring literal block formatting

5e7702b

Remove malformed literal block structure in TESTS section that was causing RST218 error. Properly format the doctest examples.

Fix RST218 linting error in FriCAS interface

43c7cf6

Remove TESTS:: header that was causing RST218 literal block formatting error while preserving the meaningful doctest that verifies new process starts after quit(). This addresses linting compliance while maintaining documentation quality.

cxzhong marked this pull request as ready for review August 11, 2025 12:36

github-actions bot added the s: needs review label Aug 11, 2025

Add TESTS:

b38bf8d

cxzhong closed this Aug 14, 2025

cxzhong deleted the fix-fricas-doctest-pickling branch August 14, 2025 07:11

github-actions bot removed the s: needs review label Aug 14, 2025

cxzhong mentioned this pull request Oct 19, 2025

Add __reduce__ method to LazyImport class for pickle support #41054

Closed

5 tasks

cxzhong restored the fix-fricas-doctest-pickling branch October 20, 2025 06:44

cxzhong reopened this Oct 20, 2025

Merge branch 'develop' into fix-fricas-doctest-pickling

e44838f

github-actions bot added the s: needs review label Oct 20, 2025

cxzhong added s: positive review and removed s: needs review labels Oct 20, 2025

user202729 added s: needs work s: positive review and removed s: positive review s: needs work labels Oct 20, 2025

vbraun merged commit 31089d4 into sagemath:develop Oct 27, 2025
24 of 26 checks passed

github-actions bot removed the s: positive review label Oct 27, 2025

Uh oh!

Fix fricas doctest pickling #40570

Fix fricas doctest pickling #40570

Uh oh!

Conversation

cxzhong commented Aug 11, 2025

📝 Checklist

⌛ Dependencies

Uh oh!

github-actions bot commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tscrim commented Aug 13, 2025

Uh oh!

cxzhong commented Aug 13, 2025

Uh oh!

tscrim commented Aug 13, 2025

Uh oh!

cxzhong commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tscrim commented Aug 18, 2025

Uh oh!

cxzhong commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cxzhong commented Oct 20, 2025

Uh oh!

cxzhong commented Oct 20, 2025

Uh oh!

nbruin commented Oct 20, 2025

Uh oh!

user202729 commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlitzky commented Oct 20, 2025

Uh oh!

user202729 commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbruin commented Oct 20, 2025

Uh oh!

tscrim commented Oct 21, 2025

Uh oh!

tscrim commented Oct 21, 2025

Uh oh!

nbruin commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tscrim commented Oct 21, 2025

Uh oh!

nbruin commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

user202729 commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

github-actions bot commented Aug 11, 2025 •

edited

Loading

cxzhong commented Aug 13, 2025 •

edited

Loading

cxzhong commented Aug 18, 2025 •

edited

Loading

user202729 commented Oct 20, 2025 •

edited

Loading

user202729 commented Oct 20, 2025 •

edited

Loading

nbruin commented Oct 21, 2025 •

edited

Loading

nbruin commented Oct 21, 2025 •

edited

Loading

user202729 commented Oct 21, 2025 •

edited

Loading