kornia / tutorials

Repository containing the Kornia related tutorials
https://kornia.github.io/tutorials/
Apache License 2.0
45 stars 32 forks source link

Refactor tutorials #41

Closed johnnv1 closed 1 year ago

johnnv1 commented 1 year ago

The goal here is to have an CI for this repo, and also a better organization to keep maintainability

motivation from https://github.com/kornia/kornia/issues/2171

Changes

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

edgarriba commented 1 year ago

i will review soon

edgarriba commented 1 year ago

@johnnv1 is a bit complicated to review without the outputs. Have you verified that the outputs are correct ?

johnnv1 commented 1 year ago

@johnnv1 is a bit complicated to review without the outputs. Have you verified that the outputs are correct ?

hmmm, yes I have checked -- but my idea here with nbstripout on pre-commit is to remove all outputs, in this manner the repo should get lighter (so it will be a little faster to download and setup on kornia CI). But that way we would have no way of checking if the tutorial is correct, so I will try to think of another way to do it...

On second thought now, I also don't think it will make much difference compared to the time it takes to setup the pytorch with conda

johnnv1 commented 1 year ago

I think the windows test are failing because can't download successfully the images with the wget on the notebooks

ducha-aiki commented 1 year ago

hmmm, yes I have checked -- but my idea here with nbstripout on pre-commit is to remove all outputs,

@johnnv1 if we do so, we have to somehow increase readthedocs limits for running cells. Heavy stuff like LoFTR https://kornia-tutorials.readthedocs.io/en/latest/image_matching.html is actually crashing on readthedocs because of timeout, and if we would strip the outputs, users would not see any final image there.

johnnv1 commented 1 year ago

is actually crashing on readthedocs because of timeout

if we build on CI, deploy to ghpages (just push to the branch), and then in the readthedocs build we just clone that branch... would it work?

johnnv1 commented 1 year ago

or, we keep it, the notebooks with their outputs, and we can disable the execution of the notebooks in the tutorial build

--- a/tutorials/conf.py
+++ b/tutorials/conf.py

+# -- myst_nb options --------------------
+nb_execution_mode = 'off'

and i think, this way we can have the preview on PR's too


i found out the time limit per cell of the myst-sb is 30s as default...

you think this time limit crash -- is from the config of the myst-sb or it's a limitation from readthedocs?

johnnv1 commented 1 year ago

you think this time limit crash -- is from the config of the myst-sb or it's a limitation from readthedocs?

I tested it here, seems to be just the myst config:

but the read docs timeout is 15min, this goes well, but for things like the lightning tutorial where a model is trained (heavy stuff) we can just skip the running adding it to myst config to ignore the execution of these one

johnnv1 commented 1 year ago

I replaced wget with a python function to download the data, and also run pre-commit for all files -- probably the easy way to check this is looking here: https://johnnv1-kornia-tutorials.readthedocs.io/en/latest/

It's a clone of this branch deployed to readthedocs just to test

johnnv1 commented 1 year ago

I don't have a windows env, either time to try to fix the bugs in windows, so I just removed windows from the CI and will open an issue if someone wants to fix this later

johnnv1 commented 1 year ago

And I have no idea why the lighting tutorial is getting stuck in some thread (CI with python 3.10 and macOS) 😄

johnnv1 commented 1 year ago

@edgarriba can you add https://pre-commit.ci here? 😄