-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Pyston workflow #368
base: master
Are you sure you want to change the base?
Pyston workflow #368
Conversation
mathics/builtin/numbers/calculus.py
Outdated
Expression(SymbolPlus, *expansion), | ||
Expression(SymbolPower, Expression(SymbolO, variable), powers[-1]), | ||
Expression(SymbolPower, Expression("O", variable), powers[-1]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a regression, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that. I missed it in the rebase. I think now it is like in master
@mmatera All of this is good and I'd like to see this in, but it really should be done in 3 separate smaller PRs
Doing this makes it easier to for someone like me to independently test the pieces and in the future make it easier to track activity. |
Actually, I tried to do that. #362 contains the two first bullet points, adding the third one in this PR. |
As for #362, the concerns there weren't addressed. When I tried this it wasn't working and gave a traceback showing the results I got. Given this please split off the builtin changes to calculus.py and algorithm/optimizers.py which is an independent thing from the doctest changes and let's get that in. Then with the narrowed #362 I will try testing that again to see what went wrong. It could be something in my setup but it also might not. After that, then rebase this on the parts that don't include #362. I understand the desire and tendency of mixing separable (but causally related) and sometimes unrelated things in a single PR. I tend to do this too to try to move things along faster. However, as soon as there is a problem with any of this, then one needs to go back and split things up. Thanks. |
@rocky, here you can see the two pieces composing #362:
which fails in different places. Together, tested against the basic installation, they pass all the tests. On the top of that, I added the pyston workflow that you can see here. On the other hand, I was not able to reproduce your traceback. This is why I introduced the ubuntu-minimal workflow. |
Neither of these addresses the changes to NIntegrate in isolation: to expand testing, and to remove the requirement of scipy on that. I have that split out into #374 If we can agree on this, then let's merge that in. It is a well defined piece of work that stands on its own.
Ok - but it is sad that we have to go through all of this discussion and additional PR's before you report that you were not able to reproduce the traceback. It is likely a problem in my environment or on my side. But it is something I'd like to understand and resolve. It may also be that more work is needed in a particular area and we decide to do that in a different PR. But again, I'd like to understand what's up here. So want to try again, but with a smaller and more isolated PR. So after #374 goes through, let's just do the check_requires part in doctest and moved to core part by itself - no need to add CI testing with Pyston into that part. That can be a separate PR. |
@rocky, thanks for this.
Good. My problem was that I couldn't test this in a fresh environment, without adding the workflow. But OK, let´ s merge it as it is in #374.
I thought that adding the ubuntu-minimal.yml workflow was enough to show that. In any case, I am not sure about what is failing in your environment.
OK, but that part was already isolated. Once the other parts are ready, we can just add the workflow for Pyston. |
All I did was remove scipy in 3.8.13 and test:
|
@mmatera look at https://github.com/Mathics3/mathics-omnibus/tree/5.0.0.dev0/docker/src for pre-built wheels that can be used to speed up pyston CI which currently takes so long. I don't know right now what the best way to manage the pyston wheels is though. |
71081e7
to
93ed59e
Compare
@mmatera what is the status of this PR? |
It needs to be redone using prebuilt packages to make running CI not take a long time to run. |
How can that be done? |
Here #368 (comment) it is supposed to be a clue, but I still didn't try to dig on it. |
I managed to pick the pre-built wheels from mathics-omnibus, and install them during the CI. However, for some reason,
When you have time, could you please check if there is a problem with this file? |
0854926
to
04e2416
Compare
@rocky, I had to hack a little bit an older version of scipy, but now it works. The next time you built mathics-omnibus, we can change the link to the wheel. |
I think I ran into the same thing in reworking the docker file for more current sources. I had since removed that wheel, and for simplicity I now just stick to one version - Python 3.10.12 for everything. |
OSX is taking over 7 minutes to run and Windows is taking over 11 minutes. These times are too long to be used in our normal CI workflow. |
Again, this is a reason to split mathics-core into a true "core" and move specialized builtins to different external modules. In any case, it worth to look at what is taking too much time. |
That is a possibility, but it is not the only one. There are other things that can be done that require less effort. Currently 1-2 minutes is taken in just installing OS dependencies, and here, so not having to build packages over and over again helps. But most of the time is in testing. So running tests in parallel based on modules can speed things up. But come to think of it, that option is available now, but using options to docpipeline and pytest. However when autoloading is done, we have the possibility of having a more transparent way to separate WMA builtins that rely on special libraries like image libraries. While yes, splitting up core would be nice, the remark that it is going to fix the problems in testing are glib. I mentioned that getting this done and done properly is going to take time. But that aside, he overall computer time will be the same (actually, probably more since there is duplication in setup), but we'll be able to do more in parallel. Having more CI runs increases complexity a little. I believe that in the free CI tier the overall CPU time is counted and once we go over that, then new CI jobs are not allowed to run. So again, parallelizing testing while it speeds things up in elapsed time, does not as directly address the problem as say reducing the OS and package setup time. |
@mmatera I see that I was confused about things. If the other OS's were slow, then that is slowness that is in master as well! The CI code is the same as in master. My apologies here. |
wget https://github.com/Mathics3/mathics-omnibus/raw/mathics-6.x/docker/src/matplotlib-3.5.2-pyston38-pyston_23_x86_64_linux_gnu-linux_x86_64.whl | ||
wget https://github.com/Mathics3/mathics-omnibus/raw/mathics-6.x/docker/src/PyWavelets-1.3.0-pyston38-pyston_23_x86_64_linux_gnu-linux_x86_64.whl | ||
# Scipy wheel does not work.... | ||
wget www2.fisica.unlp.edu.ar/~matera/scipy-1.7.3-pyston38-pyston_23_x86_64_linux_gnu-linux_x86_64.whl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmatera Please copy scipy-1.7.3 into mathics-omnibus/docker/src and adjust the link here. Many thanks.
Never mind. 1.7.3 is rather old. I am building scipy 1.10.1 now and will add this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmatera I can't seem to build a 1.10.1 that will work in CI. So back to try adding yours to mathics-omnibus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I can't get copying 1.7.3 over to mathics-omnibus working either. So back to getting it from www2.fisica.unlp.edu.ar/~matera/scipy-1.7.3-pyston38-pyston_23_x86_64_linux_gnu-linux_x86_64.whl
Perhaps down the line we can set up a cache https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows.
This was my mistake. If it is slow, it is also slow in master
6a792b4
to
c83cb8f
Compare
2d095a9
to
5fef508
Compare
I am okay with merging this now (even if I am not enthusiastic about it). @mmatera merge whenever you are satisfied. |
This PR adds a basic Pyston workflow.
Something that is not still working is pytest, since it seems to use Python and not Pyston to run the tests.