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

load mathics.builtin.box #62

Merged
merged 1 commit into from
Nov 26, 2021
Merged

load mathics.builtin.box #62

merged 1 commit into from
Nov 26, 2021

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 10, 2021

This PR fixes some flakiness I found during the Definitions refactoring: mathics.builtin.box.* was not loaded during the startup.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 25, 2021

@rocky, please confirm to me that this was a mistake and that we have no good reasons to do not to load these modules.

@rocky
Copy link
Member

rocky commented Nov 25, 2021

@rocky, please confirm to me that this was a mistake and that we have no good reasons to do not to load these modules.

I think all of those are imported as a result of importing other builtins via Python import statements. Was there a particular Builtin that wasn't found?

Because this is slow and non-standard, we use the dynamic loading only when there the modules wouldn't have otherwise have been imported by using Python's normal import mechanism.

And that is yet another good reason why it is nice to separate top-level Builtins from the implementation and the boxing of those and the formatting - none of the things in the later category needs to be loaded in a non-standard way.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 25, 2021

@rocky, please confirm to me that this was a mistake and that we have no good reasons to do not to load these modules.

I think all of those are imported as a result of importing other builtins via Python import statements. Was there a particular Builtin that wasn't found?

Yes, all the built-in inside mathics.builtin.box are not loaded. I discovered this playing with the Definitions object.

Because this is slow and non-standard, we use the dynamic loading only when there the modules wouldn't have otherwise have been imported by using Python's normal import mechanism.

Indeed, at some point we need to thing about a better mechanism to load de built-in definitions. But it is a lot of work, and it just save a few seconds in the loading time.

And that is yet another good reason why it is nice to separate top-level Builtins from the implementation and the boxing of those and the formatting - none of the things in the later category needs to be loaded in a non-standard way.

OK, but again, that is another step. My idea, in the long term, is to put all the complex formatting stuff in external modules (like pymathics-asy or pymathics-matplotlib and keep in the core the essential infrastructure for evaluation and rule application. In any case, we still have a long trip until there.

@rocky rocky merged commit 1af364e into master Nov 26, 2021
@rocky rocky deleted the load-builtin-box branch May 28, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants