Opened 3 years ago

Closed 3 years ago

#21513 closed enhancement (fixed)

Package rst2ipynb

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-7.4
Component: packages: optional Keywords: days79
Cc: slabbe, nthiery, boussica, ​vdelecroix Merged in:
Authors: Thierry Monteil, Nicolas M. Thiéry Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: d5d5680 (Commits) Commit: d5d5680d547487a9702758f4a62852f34a0ab725
Dependencies: #21512 Stopgaps:

Description (last modified by nthiery)

rst2ipynb converts rst source to ipynb worksheet.

This package will be used through the straightforward sage -rst2ipynb command, see #21514 You can still test it by hand as follows: install pandoc on your system and if you have a myfile.rst do:

sage -sh
rst2ipynb --kernel='sagemath' myfile.rst -o myfile.ipynb

Change History (34)

comment:1 Changed 3 years ago by tmonteil

  • Dependencies changed from 21512 to #21512

comment:2 Changed 3 years ago by slabbe

  • Cc slabbe added

comment:3 Changed 3 years ago by slabbe

Last edited 3 years ago by slabbe (previous) (diff)

comment:4 follow-up: Changed 3 years ago by slelievre

Last edited 3 years ago by slelievre (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 3 years ago by tmonteil

Replying to slelievre:

Or do you have https://github.com/nthiery/rst-to-ipynb in mind?

Indeed. But i proposed some changes to allow tables to be correctly displayed and the possiblity to tune the jupyter kernel (we need the sagemath one), now i have to wait that the repository get stablized again to push my changes on this ticket with the most recent version/hash, in particular, the work is not licensed yet, and should become BSD-3clause in case jupyter wants to include it.

comment:6 Changed 3 years ago by tmonteil

  • Branch set to u/tmonteil/package_rst-to-ipynb

comment:7 Changed 3 years ago by tmonteil

  • Commit set to 45e561a5c9e1b8665d2899c589df5ae476da3941
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

6aaed41#21490 package pandoc_attributes
f062c3dMerge branch 'u/tmonteil/package_pandoc_attributes' of trac.sagemath.org:sage into HEAD
e1ea92b#21512: package notedown
45e561a#21513: package rst-to-ipynb

comment:8 Changed 3 years ago by git

  • Commit changed from 45e561a5c9e1b8665d2899c589df5ae476da3941 to 966a95d9b6951543251a7d0942caf098d58c2c0f

Branch pushed to git repo; I updated commit sha1. New commits:

966a95d#21513 : rst_to_ipynb -> rst-to-ipynb

comment:9 Changed 3 years ago by tmonteil

As suggested by Samuel by email, the name of the upstream package is rst-to-ipynb, with scores.

comment:10 Changed 3 years ago by tmonteil

ping ?

comment:11 Changed 3 years ago by slabbe

  • Status changed from needs_review to needs_work

After downloading the zip file into upstream directory, both make rst-to-ipynb and sage -f rst-to-ipynb fail:

$ sage -f rst-to-ipynb
make build/make/Makefile
make[1] : on entre dans le répertoire « /home/labbe/Applications/sage-git »
make[1]: « build/make/Makefile » est à jour.
make[1] : on quitte le répertoire « /home/labbe/Applications/sage-git »
build/bin/sage-logger \
	"cd build/make && ./install 'all-toolchain'" logs/install.log
Nothing to (re)build / all up-to-date.

Error: package 'rst-to-ipynb' not found
Assuming it is an old-style package... (this is deprecated: use -p instead of -i to install old-style packages)

Attempting to download package rst-to-ipynb
>>> Checking online list of optional packages.
>>> Checking online list of experimental packages.
>>> Checking online list of huge packages.
Error: could not find a package matching rst-to-ipynb
       Try 'sage --package list' to see the available packages
       Did you mean: rst_to_ipynb?

comment:12 Changed 3 years ago by slabbe

Also make rst_to_ipynb fail:

[rst_to_ipynb-fd58cd03] Found local metadata for rst_to_ipynb-fd58cd03
[rst_to_ipynb-fd58cd03] Attempting to download package rst_to_ipynb-fd58cd03.zip from mirrors
[rst_to_ipynb-fd58cd03] http://www.mirrorservice.org/sites/www.sagemath.org/spkg/upstream/rst_to_ipynb/rst_to_ipynb-fd58cd03.zip
[rst_to_ipynb-fd58cd03] [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
[rst_to_ipynb-fd58cd03] ERROR [transfer|run:135]: [Errno 404] Not Found: '//www.mirrorservice.org/sites/www.sagemath.org/spkg/upstream/rst_to_ipynb/rst_to_ipynb-fd58cd03.zip'
[rst_to_ipynb-fd58cd03] http://www-ftp.lip6.fr/pub/math/sagemath/spkg/upstream/rst_to_ipynb/rst_to_ipynb-fd58cd03.zip
[rst_to_ipynb-fd58cd03] [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
[rst_to_ipynb-fd58cd03] ERROR [transfer|run:135]: [Errno 404] Not Found: '//www-ftp.lip6.fr/pub/math/sagemath/spkg/upstream/rst_to_ipynb/rst_to_ipynb-fd58cd03.zip'

...

comment:13 Changed 3 years ago by slabbe

The description of the ticket should be updated: scores vs underscores.

Can you add information in the ticket how I should use that package? Inside sage? Inside sage -sh? Using sage -rst2ipynb ? What command should I type?

comment:14 Changed 3 years ago by tmonteil

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Package rst_to_ipynb to Package rst-to-ipynb

The ticket description said that the zipball should be renamed as rst_to_ipynb-fd58cd03.zip or directly downloaded from my homepage, anyway i updated the description to be more explicit and added hints for testing the package.

comment:15 follow-up: Changed 3 years ago by slabbe

Thanks for the info. I forgot about #21514.

I managed to run make rst_to_ipynb succesfully from scratch on another machine.

I did some testing... Sage blocks like the following works great:

sage: 4+4
8
sage: for i in range(5):
....:     print i
0
1
2
3
4

They are translated into one input block + one output block. Great.

I get a problem with

>>> 4+4
8
>>> for i in range(5):
...     print i
0
1
2
3
4

which get written into one input block. Is it possible to patch Nicolas Thiéry package?

comment:16 in reply to: ↑ 15 Changed 3 years ago by tmonteil

  • Cc nthiery added

Replying to slabbe: [...]

I get a problem with

>>> 4+4
8
>>> for i in range(5):
...     print i
0
1
2
3
4

which get written into one input block. Is it possible to patch Nicolas Thiéry package?

I hope so ! Just make patch a send him a pull-request. I did that already to be able to define the kernel (so that the sage -rst2ipynb command builds sagemath worksheets), and to fix malformatted tables.

comment:17 Changed 3 years ago by slabbe

Ok! I will do this probably next week during the sage days.

Question maybe for Nicolas. Should I patch the sageblockfilter.py file or should I create a new file pythonblockfilter.py ?

comment:18 Changed 3 years ago by tmonteil

Please note that contributing to rst-to-ipynb is independent of the current ticket, which is about packaging it. It would be nice to have sage -rst2ipynb available soon for everyone (it is ready for almost 3 months now).

Note also that such a sage command should make your own development of rst-to-ipynb easier ;) (i used it massively for scripting the build of tutorials last month, it is very handy).

comment:19 Changed 3 years ago by nthiery

Hi Sébastien,

Shall we have a small sprint on this this week?

Cheers,

Nicolas

comment:20 Changed 3 years ago by nthiery

Hi Thierry,

Adrien tried to use rst-to-ipynb by cloning the repo; that was painful to use for him (getting the dependencies, ...). So I ended up making it into a proper Python/pip package, which installs the conversion script. While I am at it, I might as well upload the package on pipy (planning to do this tomorrow).

At this stage, the script is called rst2ipynb for consistency with sage -rst2ipynb and many existing rst2xxx converters.

Questions:

  • Should the pip package be called rst2ipynb or rst-to-ipynb?
  • In the former case, should the github repo be renamed rst2ipynb?
  • Once we have a proper pip package on pipy, do we need a Sage package? (if no, sorry for the lack of sync; we could have saved the writing of this package).

Cheers,

Nicolas

comment:21 Changed 3 years ago by tmonteil

Let me just remind that Sage is a collective effort, which aims to share the development of common features in order to focus on the code specific to our own research.

All the sage -rst2ipynb stuff (packaging, scripting,...) is ready for 3 months now (i wrote those in August during https://wiki.sagemath.org/days75).

If, instead of trying alternate/duplicate ways to install it painlessly, those tickets had been reviewed, sage -rst2ipynb command would have been in Sage 7.4 already and Adrien would not even have had to clone anything nor to suffer some dependencies issues, because some Sage developers (myself and the reviewers) would have taken care of this once for all.

Now, could please someone review the last 2 tickets (this one and #21514 that i will push over this branch after it is reviewed) so that sage -rst2ipynb command will be available to every Sage user in the next release, which might end up in real-life interesting feature requests and useful improvements.

I presume Sebastien noticed the >>> issue because he used the package on a concrete use case that led to errors. The rst-to-ipynb package development will benefit from its easy availability to every Sage users, each with their specific workflow.


That said, regarding pip installation, there are two very different things: having rst-to-ipynb being pip-installable, and having it hosted/mirrored on PyPI.

If you write a convenient setup.py script, we could modify the spkg-install script to use the pip command for the install. But please do not condition the review of this ticket to that, plan it for 7.6 instead, or earlier if the last 2 ticket get merged soon.

Now, regarding fetching from PyPI for Sage, it is another story, since rst-yo-ipynb depends on notedown and pandoc_attributes, whose versions on PyPI are bitroting a lot behind the github source from which the Sage packages are built. Changes since the last PyPI update (more than 1+1/2 year) include Python3 support and various fixes.

If you have enough free time for not only creating, but also being commited to maintaining a PyPI rst-to-ipynb package indefinitely, it is definitely interesting.

It could even replace the current spkgs on the longer run, but it requires first to pressure notedown/pandoc_attributes dev to also update his PyPI repositories, and probably he does not have much time for this.

Moreover, regarding the development of rst-to-ipynb, i guess it is better to develop and test it against the latest versions of its dependencies, and not the PyPI ones, since in case of an upstream problem, it is much more doable to obtain a pull than to obtain a new PyPI release.

That said, having a somehow degraded (because of older dependencies) version on PyPI could be interesting for non-Sage users, but it would currently be a loss for Sage, and since the packaging work with up-to-date dependencies is done anyway, i do not see any benefit in relying on older Python3 incompatible versions.

This is only a very personal suggestion, but if i had such free time, i would better work on having a rst2ipynb feature being part of Jupyter (which is why i promoted the licensing of rst-to-ipynb to be the same as the jupyter one), so that it will be much more maintained, widely tested, used in different contexts, hence improved. It would perhaps imply finding a non-pandoc way to do the conversion, i am not sure.

Regarding the naming scheme, it is not very important, and certainly not imposed by the sage -rst2ipynb command name, since this later needs a dedicated script anyway (being interfaced with the sage command and specifying the Sage kernel).

Please, let us focus on having a public working sage -rst2ipynb command first. It is a key step for a lossless sagenb to jupyter migration.

comment:22 Changed 3 years ago by tmonteil

  • Cc boussica ​vdelecroix added

comment:23 Changed 3 years ago by nthiery

Salut Thierry,

With Sébastien, we are precisely in the process of reviewing this ticket and its sage -rst2ipynb counterpart, with goal to get it done hopefully today. That's what led me to try first a manual install of rst2ipynb, and realize it was even more messy than I thought. Making it pip installable / a pipy package is part of that collective effort to work with upstream and write stuff useful to the larger community :-)

I agree, it would have been more productive to discuss earlier on the best approach for packaging.

Thanks for pointing out the caveat about the outdated versions of notedown on pipy. Plus the necessity to pass down extra arguments for the kernel. This is indeed a good reason to have a sage package, at least for now.

My plan:

  • Do tiny further edits to rst2ipynb, and make an "official release". (it's already pip installable)
  • Release it on pipy (that's cheap anyway)
  • Update the Sage package to use pip to install it from a source clone.

For the long run: having a reST to ipynb converter under the Jupyter umbrella would indeed be best. The current pandoc+notedown approach is reaching its limits (e.g. for handling cross references, in particular for multi-file documents). I believe more in a Sphinx-based approach. In a discussion with the Jupyter dev, they pointed out that pandoc was a heavy dependency, and that Sphinx also looked better to that respect.

Cheers,

Nicolas

comment:24 Changed 3 years ago by nthiery

rst2ipynb 0.2.1 "released" and uploaded on pipy: https://pypi.python.org/pypi/rst2ipynb/0.2.1

comment:25 Changed 3 years ago by nthiery

  • Branch changed from u/tmonteil/package_rst-to-ipynb to u/nthiery/package_rst-to-ipynb

comment:26 Changed 3 years ago by nthiery

  • Commit changed from 966a95d9b6951543251a7d0942caf098d58c2c0f to d44379de829bdc001554e0d58e78a3f05d5eaf90
  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Package rst-to-ipynb to Package rst2ipynb

New commits:

d44379dMerge branch 'develop' into t/21513/package_rst-to-ipynb

comment:27 Changed 3 years ago by git

  • Commit changed from d44379de829bdc001554e0d58e78a3f05d5eaf90 to ade0a42757aa70f4d1e647933ee8c14fe1461c21

Branch pushed to git repo; I updated commit sha1. New commits:

ade0a42Refactor the rst2ipynb package to use the new pip-installable package

comment:28 Changed 3 years ago by nthiery

  • Status changed from needs_work to needs_review

Done! Thierry, Sébastien, do you mind reviewing my changes?

I am now moving on to #21514.

comment:29 Changed 3 years ago by nthiery

  • Reviewers set to Sébastien Labbé, Nicolas M. Thiéry

Not sure whether I should be author or reviewer here :-)

comment:30 follow-up: Changed 3 years ago by tmonteil

The way you did the refactor lost all the history so it is hard to see what changed, you should use git mv command, then modify the files, then git add command.

I have to recompile parts of Sage, so the rest might take time.

comment:31 in reply to: ↑ 30 Changed 3 years ago by slabbe

I am sorry that I was late at working on this ticket this Fall.

I agree to postpone the fix of >>> blocks to a later release of rst2ipynb.

I intend to review this ticket so that it gets a positive review *during* Sage Days 79.

Replying to tmonteil:

The way you did the refactor lost all the history so it is hard to see what changed, you should use git mv command, then modify the files, then git add command.

+1

Nicolas, can you redo your last commit and use git mv instead (you will need to git push -f your new branch: whatever).

comment:32 Changed 3 years ago by git

  • Commit changed from ade0a42757aa70f4d1e647933ee8c14fe1461c21 to d5d5680d547487a9702758f4a62852f34a0ab725

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

421388a21513: Refactor the rst2ipynb package to use the new pip-installable package, step 1: renaming the pkg directory
d5d568021513: Refactor the rst2ipynb package to use the new pip-installable package, remainder

comment:33 Changed 3 years ago by slabbe

  • Authors changed from Thierry Monteil to Thierry Monteil, Nicolas M. Thiéry
  • Keywords days79 added
  • Reviewers changed from Sébastien Labbé, Nicolas M. Thiéry to Sébastien Labbé
  • Status changed from needs_review to positive_review

comment:34 Changed 3 years ago by vbraun

  • Branch changed from u/nthiery/package_rst-to-ipynb to d5d5680d547487a9702758f4a62852f34a0ab725
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.