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

Set up github actions and jupyterlite #486

Closed
wants to merge 20 commits into from
Closed

Conversation

moble
Copy link
Member

@moble moble commented Aug 9, 2023

  1. Added a github actions workflow that creates a jupyterlite instance with all of the ipynbs from the examples directory, including the tutorial_algebra.ipynb at the top level.

    Sympy Live now works through (a modified version of) jupyterlite, which runs entirely in the browser, with no requirement on the user to install anything. Galgebra has lots of ipython notebooks that could be really nice as "live" documentation in a similar way. This PR sets that up. The process is just a quick (~1 minute) Github Actions run, which will then deploy to the (currently unused) Github Pages for this repo.

    You can see a preview from my fork: https://moble.github.io/galgebra/jupyterlite

  2. Added a github actions workflow that attempts to replace CircleCI for testing and deployment

    A previous attempt to change one little thing led to finding out that CircleCI fails at the very first step now.

  3. I had to fix a few little things to get tests to pass:

    • I got an error when trying to run setup.py with a current version of setuptools. It was apparently a bug to allow the syntax python_requires=">=3.6.*": reviews.llvm.org/rLNT0476dede37838e2f765f50dd26734cfced9d90d9. I've updated that.
    • inspect.getargspec was deprecated, so its use in Mlt.__init__ had to be corrected.
    • The next branch in Mlt.__init__ was referring to a non-existent args variable, so I just set it to 0.

Some notes:

  • Someone will need to go to https://github.com/pygae/galgebra/settings/secrets/actions and add appropriate values for PYPI_API_TOKEN and CODECOV_TOKEN.
  • Unfortunately, the pyodide kernel makes it impossible to pre-install any special requirements (other than things that already ship with pyodide, which includes sympy). So, every notebook has to start with [%pip install galgebra]. This could maybe be worked around using the xeus-python backend, but would require waiting until conda-forge gets the updated version before building jupyterlite.
  • This would probably be a good time to re-organize the examples directory. In particular, a more specific set of notebooks with more complete tutorials might be helpful — stepping through 3D GA with relation to quaternions, STA, CGA, etc.
  • There were two settings I had to change / check on this repo to make sure the page can deploy:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }}
run: |
python setup.py sdist bdist_wheel
twine upload dist/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and it uses twine under the hood

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be done in a separate, smaller follow-up PR. Just checked that my CircleCI script was also using twine, it just has ported here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah. That's actually what I use for my other python packages. But now I see that they've actually updated to prefer "Trusted publishing" over API keys, which would require more manipulation by someone with PyPI access. So yeah, maybe leave it for a follow-up PR. :)

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! I've not looked at galgebra for some time, and it's not surprising that CI is grinding to a halt.

I think I'd prefer to merge the move to GH actions as a standalone PR; I'm happy to make the split myself if you don't want to.

Regarding jupyterlite: I was under the impression that we had the notebooks set up to run in Binder already, but I must have been thinking of a different project.

@utensil
Copy link
Member

utensil commented Aug 10, 2023

Thank you for doing this! It's a comprehensive polish of GAlgebra, clearly you have put into a lot of efforts into this, the PR is carefully prepared, aligned and documented.

The old CircleCI is obsolete, and indeed should be updated to Github Actions. And it's nice to know pyodide has evolved to support something like jupyterlite.

Personally, I'm OK with merging this PR almost as is, including the move to Github Actions and the addition of jupyterlite, then improve based on that. I like jupyterlite (browser based) as much as Binder (docker based, slow to start up). The inclusion of jupyterlite can be considered an experimental feature.

Review in progress, more comments coming.

@utensil
Copy link
Member

utensil commented Aug 10, 2023

I've just added them.

  • every notebook has to start with [%pip install galgebra].

Do you mean the users need to manually execute %pip install galgebra before running the notebook ? I tried this and it works (without []).

It's better to save the trouble for the users, and add it via CI into the notebooks, is it possible?

  • This would probably be a good time to re-organize the examples directory. In particular, a more specific set of notebooks with more complete tutorials might be helpful — stepping through 3D GA with relation to quaternions, STA, CGA, etc

More examples welcome! And you're also welcome to add features to GAlgebra in the ways that suit your need if you're motivated to maintain these features.

May I ask what led you to GAlgerba and what's your use case of it?

There were two settings I had to change / check on this repo to make sure the page can deploy:

They're both set (presumably by Eric).

@utensil
Copy link
Member

utensil commented Aug 10, 2023

  • A previous attempt to change one little thing led to finding out that CircleCI fails at the very first step now.

It's wierd. The cause is git@github.com: Permission denied (publickey). But it seems to be using a valid ssh key set up by @eric-wieser (I no longer have memory about how I set it up 🤦 ):

image image

Anyway, maybe we don't need to bother with debugging this and just make sure the new CI passes tests: https://github.com/moble/galgebra/actions/runs/5814032932/job/15762888427

@utensil
Copy link
Member

utensil commented Aug 10, 2023

  • Unfortunately, the pyodide kernel makes it impossible to pre-install any special requirements (other than things that already ship with pyodide, which includes sympy)

According to Ship additional Pyodide wheels at build time + Using a custom Pyodide distribution, it seems feasible?

@moble
Copy link
Member Author

moble commented Aug 10, 2023

I think I'd prefer to merge the move to GH actions as a standalone PR; I'm happy to make the split myself if you don't want to.

I have no problem with that. I'm coming close to the end of my spare time, though, so I'd appreciate it if you could make the split.

Do you mean the users need to manually execute %pip install galgebra before running the notebook ? I tried this and it works (without []).

Yeah, at least before the first import galgebra. You can see a working example on my fork, in the tutorial notebook.

It's better to save the trouble for the users, and add it via CI into the notebooks, is it possible?

For now, I've manually added it to the tutorial notebook, and just copied all the other notebooks to the examples directory as they are. I don't know how robust it would be to try this automatically via CI.

May I ask what led you to GAlgebra and what's your use case of it?

I'm actually an OG user, from back in the days when brombo was just starting out with it. I've developed a number of specialized quaternion packages and a few STA packages mostly for numerics, related to analysis of gravitational waves. Occasionally I use galgebra to do analytical work / check formulas that I'm implementing elsewhere. But quite generally, I'm a big proponent of geometric algebra, so I'm happy to help this package thrive however I can.

There were two settings I had to change / check on this repo to make sure the page can deploy [...]

They're both set (presumably by Eric).

No, I did it; I'm an owner in pygae. I just wanted to point out those two settings for posterity, and in case anyone objects for any reason.

According to Ship additional Pyodide wheels at build time + Using a custom Pyodide distribution, it seems feasible?

I am actually already building and shipping the galgebra wheel in the jupyterlite workflow, but the user still has to run %pip install galgebra; it just uses the version of galgebra that was in git at the time the jupyterlite workflow ran, rather than fetching from PyPI.

I have to admit that I don't actually understand the second link, or whether it would allow the user to skip the %pip command. It looks complicated, but I'm certainly open to suggestions.

Another approach that could work might be getting galgebra added to the packages included with pyodide. (I don't know how particular they are about that list...)

@utensil
Copy link
Member

utensil commented Aug 11, 2023

May I ask what led you to GAlgebra and what's your use case of it?

I'm actually an OG user, from back in the days when brombo was just starting out with it. I've developed a number of specialized quaternion packages and a few STA packages mostly for numerics, related to analysis of gravitational waves. Occasionally I use galgebra to do analytical work / check formulas that I'm implementing elsewhere. But quite generally, I'm a big proponent of geometric algebra, so I'm happy to help this package thrive however I can.

Cool! Excited to know! I'll check your other works too. I'm also code in Julia occasionally and to do analytical work / check GA formulas e.g. here and here are some more thoughts, though I have little spare time to work on this recently.

There were two settings I had to change / check on this repo to make sure the page can deploy [...]

They're both set (presumably by Eric).

No, I did it; I'm an owner in pygae. I just wanted to point out those two settings for posterity, and in case anyone objects for any reason.

👍

It's better to save the trouble for the users, and add it via CI into the notebooks, is it possible?

For now, I've manually added it to the tutorial notebook, and just copied all the other notebooks to the examples directory as they are. I don't know how robust it would be to try this automatically via CI.

I was thinking about automatically adding something to the JSON of ipynb, which is very feasible. But of course these is better to be experimented in a new PR or simply guide the user to do so would suffice.

Another approach that could work might be getting galgebra added to the packages included with pyodide. (I don't know how particular they are about that list...)

While it seems that they're not quite particular about the list and they might welcome it as long as it would be well maintained, and I'm not against adding galgebra to it, but I generally don't wish to maintain multiple distributions (conda, pyodide etc.) for a pure python package which pip install from pypi or github would just work. This adds extra work and wait time for each release for devs and users alike.

@moble
Copy link
Member Author

moble commented Aug 11, 2023

Actually, I've just realized that it would be best for the jupyterlite step to be part of the standard CI, so that it can depend on the tests passing. (We don't want to be uploading jupyterlite with the wheel for a failing galgebra.) Since that will take a little reconfiguration anyway, I think I'll just split this into two PRs myself.

@moble moble closed this Aug 11, 2023
@moble moble mentioned this pull request Aug 11, 2023
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.

3 participants