patch #639

mmatera · 2022-11-21T18:15:28Z

This is a patch that makes that the test passes. However, I think in means that the scheme proposed is not working as it should.

rocky · 2022-11-21T18:58:31Z

This is a patch that makes that the test passes. However, I think in means that the scheme proposed is not working as it should.

Right now I am using copy.copy to initialize the definitions builtins field. We may need copy.deepcopy instead. And there are bugs in the code that prevent that. #636 fixes one of the bugs found in running deepcopy. But there is another one out there.

The other thing that could be going wrong is some sort of delayed initialization that gets kicked off by importing the mathics.format. The magicness of this was also observed by this before. And that might explain why something like this happens. It might now be that this needs to be done every time but just the first time.

mmatera · 2022-11-21T19:23:32Z

This is a patch that makes that the test passes. However, I think in means that the scheme proposed is not working as it should.

Right now I am using copy.copy to initialize the definitions builtins field. We may need copy.deepcopy instead. And there are bugs in the code that prevent that. #636 fixes one of the bugs found in running deepcopy. But there is another one out there.

I added a comment in that direction.

This moves builtin-creation out of mathics.builtin and core.definitions and into a new module. I thing this is the last step before we actually move builtin creation of the Definitions initialization and into some one-time code.

and add run pytest setting MATHICS_CHARACTER_ENCODING

This isn't working but this is the idea. Another PR will contain some of the independent bugs found in this process

This moves builtin-creation out of mathics.builtin and core.definitions and into a new module. I thing this is the last step before we actually move builtin creation of the Definitions initialization and into some one-time code.

This isn't working but this is the idea. Another PR will contain some of the independent bugs found in this process

* adding comments * moving _initialize_system_definitions to a better place, and hide it. * fixing contribute in order to make work the pymathics loading mechanism without overwrite builtins

mmatera · 2022-11-22T13:29:25Z

@rocky,
I guess this is ready now: the problem was in mathics.builtin.base.Builtin.contribute: when a pymathics module got loaded, system_definitions.builtin was overwriten. This caused that if one test (test/test_custom_boxexpression) contributes with rules to in System`MakeBoxes, the following tests can not get ride of the new definitions. Now it seems it is working properly.

mmatera · 2022-11-22T13:35:22Z

Also, following #642, some imports in mathics.core from mathics.builtin were avoided.

rocky · 2022-11-22T13:40:49Z

@rocky, I guess this is ready now: the problem was in mathics.builtin.base.Builtin.contribute: when a pymathics module got loaded, system_definitions.builtin was overwriten. This caused that if one test (test/test_custom_boxexpression) contributes with rules to in System`MakeBoxes, the following tests can not get ride of the new definitions. Now it seems it is working properly.

Thanks for isolating what's wrong and working around/fixing this. I'd like to understand why when a pymathics module gets loaded, system_definitons.builtin is overwritten. That seems wrong, no?

All that code with a try block to work around this seems like a workaround for a bug or misfeature somewhere else.

mmatera · 2022-11-22T13:54:25Z

Thanks for isolating what's wrong and working around/fixing this. I'd like to understand why when a pymathics module gets loaded system_definitons.builtin is overwritten. That seems wrong, no?

The problem comes with MakeBoxes rules. When a pymathics module "contributes" with a MakeBoxes rule, before this patch, a rule is added to definitions.builtin["System`MakeBoxes"]. Since we didn't make a deep copy of definitions.builtin, the added rule ends in system_definitions.builtin["System`MakeBoxes"].

With the workaround, if a pymathics module wants to add a rule to MakeBoxes, now it adds it to a definition in definitions.pymathics.

All that code with a try block to work around this seems like a workaround for a bug or misfeature somewhere else.

The try block is one way to handle the case in which pymathics does not have already a definition for System`MakeBoxes. I could also have implemented with something as

if "System`MakeBoxes" not in definitions.pymathics:
   ....

But let's say that this is something "exceptional" (it happens at most once in the life of a Definitions object). In any case, I hope to get rid of all the related to the MakeBoxes special case as soon as possible.

rocky · 2022-11-22T15:51:46Z

The problem comes with MakeBoxes rules. When a pymathics module "contributes" with a MakeBoxes rule, before this patch, a rule is added to definitions.builtin["System`MakeBoxes"]. Since we didn't make a deep copy of definitions.builtin, the added rule ends in system_definitions.builtin["System`MakeBoxes"].

Ok - so let see if we can get deepcopy to work or narrow the copy so something like this doesn't happen.

mmatera · 2022-11-22T16:10:50Z

The question is, do we want wo modify Definitions.builtin? If not, even the copy is not necessary. On the other hand if what bothers you is the try block, it can be easily replaced with a conditional structure.

rocky · 2022-11-22T17:51:43Z

The question is, do we want [t]o modify Definitions.builtin? If not, even the copy is not necessary. On the other hand if what bothers you is the try block, it can be easily replaced with a conditional structure.

What bothers me is the layer upon layer of workaround, code and complexity because we aren't understanding the problem at the root cause or at a logical conceptual level.

Let me look at this over the weekend so I understand what's going on at a higher level.

You've given a lot of details as to what's up at an operational level, and that he helpful for understanding what is wrong at that level.

mmatera · 2022-11-22T18:42:32Z

The question is, do we want [t]o modify Definitions.builtin? If not, even the copy is not necessary. On the other hand if what bothers you is the try block, it can be easily replaced with a conditional structure.

What bothers me is the layer upon layer of workaround, code and complexity because we aren't understanding the problem at the root cause or at a logical conceptual level.

At the higher level, what is wrong is how we process and store MakeBoxes rules.

Notice also that to copy (or even deep copy) an object that should be immutable, to have several immutable copies of the same thing is already a workaround.

Here there is an experimental branch where I show this: https://github.com/Mathics3/mathics-core/tree/create-system_builtin-once-mm-ro

I have a couple of ideas about how to fix the way in which we handle MakeBoxes, but that would imply a lot of other changes that would make this harder to review.

For this reason, the strategy that I am trying to propose since some time ago is to try to replace unlocalized workarounds (the ones that imply changing something in one module to fix something in another module) with workarounds localized inside a module, or even better, inside the function that should be fixed, which would be easier to document and correct
afterwards.

Let me look at this over the weekend so I understand what's going on at a higher level.

Sure.

You've given a lot of details as to what's up at an operational level, and that he helpful for understanding what is wrong at that level.

I give many details because I think they are needed to understand what is actually going on.

rocky · 2022-11-26T16:54:40Z

mathics/builtin/base.py

+        # both, which are not going to be in the cached version that
+        # Definitions.get_definition provides. To make the new rules
+        # accesible, we need to clear the definitions cache.
+        definitions.clear_cache("System`MakeBoxes")


If this is only needed when pymathics modules are loaded, then why isn't this only in the branch of the if is_pymathics_module branch?

But then, I should duplicate lines 414 and 415 in both branches.

rocky · 2022-11-26T16:55:46Z

mathics/builtin/base.py

@@ -191,7 +192,12 @@ def __new__(cls, *args, **kwargs):
        # mathics.builtin.inout

        if kwargs.get("expression", None) is not False:
-            return to_expression(cls.get_name(), *args)
+            try:


This was my bad - should be removed.

Move system initialization module out of mathics.builtin. (Similar to PR #639) This also removes the need for Python to "partially initialize" mathics.builtin since it doesn't import one of its children

Move system initialization module out of mathics.builtin. (Similar to PR #639) This also removes the need for Python to "partially initialize" mathics.builtin since it doesn't import one of its children Merge mess

rocky added 2 commits November 21, 2022 14:29

Move code out of definitions

530e147

This moves builtin-creation out of mathics.builtin and core.definitions and into a new module. I thing this is the last step before we actually move builtin creation of the Definitions initialization and into some one-time code.

Add annotiations to some globals ...

da8580c

and add run pytest setting MATHICS_CHARACTER_ENCODING

rocky force-pushed the create-system_builtin-once branch from b4ceab7 to d8eeb84 Compare November 21, 2022 19:36

WIP - module load, and builtin create once

6436065

This isn't working but this is the idea. Another PR will contain some of the independent bugs found in this process

rocky force-pushed the create-system_builtin-once branch from d8eeb84 to 6436065 Compare November 21, 2022 19:53

rocky and others added 5 commits November 22, 2022 10:21

Move code out of definitions

c01508c

This moves builtin-creation out of mathics.builtin and core.definitions and into a new module. I thing this is the last step before we actually move builtin creation of the Definitions initialization and into some one-time code.

WIP - module load, and builtin create once

b5d5c5d

This isn't working but this is the idea. Another PR will contain some of the independent bugs found in this process

patch

37bffec

add comment

f1ccc6d

the true fix:

53300f5

* adding comments * moving _initialize_system_definitions to a better place, and hide it. * fixing contribute in order to make work the pymathics loading mechanism without overwrite builtins

mmatera force-pushed the create-system_builtin-once-mm branch from 231b06f to 53300f5 Compare November 22, 2022 13:25

rocky mentioned this pull request Nov 26, 2022

leaves->elements & Makefile... #644

Merged

rocky reviewed Nov 26, 2022

View reviewed changes

rocky force-pushed the create-system_builtin-once branch from 6436065 to 7fee71c Compare November 26, 2022 20:22

mmatera added 2 commits November 26, 2022 23:12

merge

be591cd

merge

c914823

mmatera mentioned this pull request Dec 3, 2022

Move mathics.core.builtin initialization code into system_initialize() #648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch #639

patch #639

mmatera commented Nov 21, 2022 •

edited by gitpod-io bot

rocky commented Nov 21, 2022 •

edited

mmatera commented Nov 21, 2022

mmatera commented Nov 22, 2022

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022 •

edited

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022 •

edited

mmatera commented Nov 22, 2022 •

edited

rocky Nov 26, 2022

mmatera Nov 26, 2022

rocky Nov 26, 2022

patch #639

Are you sure you want to change the base?

patch #639

Conversation

mmatera commented Nov 21, 2022 • edited by gitpod-io bot

rocky commented Nov 21, 2022 • edited

mmatera commented Nov 21, 2022

mmatera commented Nov 22, 2022

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022 • edited

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022

mmatera commented Nov 22, 2022

rocky commented Nov 22, 2022 • edited

mmatera commented Nov 22, 2022 • edited

rocky Nov 26, 2022

Choose a reason for hiding this comment

mmatera Nov 26, 2022

Choose a reason for hiding this comment

rocky Nov 26, 2022

Choose a reason for hiding this comment

mmatera commented Nov 21, 2022 •

edited by gitpod-io bot

rocky commented Nov 21, 2022 •

edited

rocky commented Nov 22, 2022 •

edited

rocky commented Nov 22, 2022 •

edited

mmatera commented Nov 22, 2022 •

edited