search

pymc-devs / pymc-examples

Examples of PyMC models, including a library of Jupyter notebooks.
https://www.pymc.io/projects/examples/en/latest/
MIT License
263 stars 218 forks source link

Remove jupytext and myst_nbs. #482

Closed twiecki closed 1 year ago

twiecki commented 1 year ago

Ref https://github.com/pymc-devs/pymc-examples/issues/440

Copying my comment from there: At this point I would argue we should remove the myst_nb files and jupytext, it's not worth the hassle and I don't see a reason for us to carry around two formats where one works, especially now that github has better jupyter NB diffs.

I don't think jupytext fits into our workflow. For example, we run all these re-formatters on the NB files which always brings them out of sync, so conflicts are almost guaranteed. And I don't think we should drop those. And jupytext does not seem to have any functionality built for this, or allow you to specify to defer to the ipynb version when in doubt. It's also not trivial to just nuke and recreate everything.

So ultimately it's a little bonus for a few people (not me), but a huge hassle and confusion for everyone.

OriolAbril commented 1 year ago

Since we added myst files, I have only opened reviewnb for the PRs that didn't have the myst available. If possible I only check myst and the actual readthedocs preview.

I haven't seen any changes in the github diffs for notebooks but I'd rather wait and test them before removing the myst files. There are several features we use in the website (like cell tags to hide outputs/inputs) that can't be reviewed in reviewnb or in the github notebook view. I won't be able to help the people submitting the PR without access to the raw markdown content (with breaklines and empty lines), or without access to the notebook or cell metadata.

We could try something like #483 for a bit. It should modify the jupytext step to make it very similar to the black one for example: it can fail but it will always solve itself. I would like to keep myst files if possible, we can get rid of them but I'd like to try some other config first.

I have no idea how pre-commit works, I have tried to read about it and I still continue to not understand it, and its combination with jupytext is even worse, so I am fully aware that the current pre-commit config is very far from ideal, but tend to think this is an issue with my choice of settings, not with the tools themselves.

canyon289 commented 1 year ago

jupytext has value for review and long term maintenance. similar to mypy and precommit in the main repo. I also believe it's largely a precommit problem

On Saturday, December 24, 2022, Oriol Abril-Pla @.***> wrote:

Since we added myst files, I have only opened reviewnb for the PRs that didn't have the myst available. If possible I only check myst and the actual readthedocs preview.

I haven't seen any changes in the github diffs for notebooks but I'd rather wait and test them before removing the myst files. There are several features we use in the website (like cell tags to hide outputs/inputs) that can't be reviewed in reviewnb or in the github notebook view. I won't be able to help the people submitting the PR without access to the raw markdown content (with breaklines and empty lines), or without access to the notebook or cell metadata.

We could try something like #483 https://github.com/pymc-devs/pymc-examples/pull/483 for a bit. It should modify the jupytext step to make it very similar to the black one for example: it can fail but it will always solve itself. I would like to keep myst files if possible, we can get rid of them but I'd like to try some other config first.

I have no idea how pre-commit works, I have tried to read about it and I still continue to not understand it, and its combination with jupytext is even worse, so I am fully aware that the current pre-commit config is very far from ideal, but tend to think this is an issue with my choice of settings, not with the tools themselves.

— Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc-examples/pull/482#issuecomment-1364568772, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXBFYODF6OKEEDSRMVLNU3WO46GVANCNFSM6AAAAAATILN67A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

twiecki commented 1 year ago

Sounds good, I mean definitely if we can keep it without it interfering with the comitting workflow too much that'd be ideal. Closing in favor of #483.