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

Package rst2ipynb #21513

Closed
sagetrac-tmonteil mannequin opened this issue Sep 16, 2016 · 43 comments
Closed

Package rst2ipynb #21513

sagetrac-tmonteil mannequin opened this issue Sep 16, 2016 · 43 comments

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Sep 16, 2016

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

Depends on #21512

CC: @seblabbe @nthiery @sagetrac-boussica @videlec

Component: packages: optional

Keywords: days79

Author: Thierry Monteil, Nicolas M. Thiéry

Branch/Commit: d5d5680

Reviewer: Sébastien Labbé

Issue created by migration from https://trac.sagemath.org/ticket/21513

@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-7.4 milestone Sep 16, 2016
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 16, 2016

Changed dependencies from 21512 to #21512

@seblabbe
Copy link
Contributor

comment:3

Is this related to
https://github.com/esc/rst2ipynb
or to
https://github.com/ariddell/rst2ipynb
?

@slel
Copy link
Member

slel commented Sep 27, 2016

comment:4

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

See also aaren/notedown#33

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 28, 2016

comment:5

Replying to @slel:

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.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 29, 2016

Branch: u/tmonteil/package_rst-to-ipynb

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 29, 2016

Commit: 45e561a

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 29, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2016

Changed commit from 45e561a to 966a95d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2016

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

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

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Sep 30, 2016

comment:9

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

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 8, 2016

comment:10

ping ?

@seblabbe
Copy link
Contributor

seblabbe commented Nov 8, 2016

comment:11

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?

@seblabbe
Copy link
Contributor

seblabbe commented Nov 8, 2016

comment:12

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'

...

@seblabbe
Copy link
Contributor

seblabbe commented Nov 8, 2016

comment:13

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?

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 8, 2016

comment:14

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.

@sagetrac-tmonteil sagetrac-tmonteil mannequin changed the title Package rst_to_ipynb Package rst-to-ipynb Nov 8, 2016
@seblabbe
Copy link
Contributor

seblabbe commented Nov 8, 2016

comment:15

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?

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 10, 2016

comment:16

Replying to @seblabbe:
[...]

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.

@seblabbe
Copy link
Contributor

comment:17

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 ?

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 23, 2016

comment:21

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.

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

comment:23

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

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

comment:24

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

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

New commits:

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

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

Changed commit from 966a95d to d44379d

@nthiery nthiery changed the title Package rst-to-ipynb Package rst2ipynb Nov 23, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2016

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2016

Changed commit from d44379d to ade0a42

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

comment:28

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

I am now moving on to #21514.

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

comment:29

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

@nthiery
Copy link
Contributor

nthiery commented Nov 23, 2016

Reviewer: Sébastien Labbé, Nicolas M. Thiéry

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 23, 2016

comment:30

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.

@seblabbe
Copy link
Contributor

comment:31

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 @sagetrac-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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 23, 2016

Changed commit from ade0a42 to d5d5680

@seblabbe
Copy link
Contributor

Changed keywords from none to days79

@seblabbe
Copy link
Contributor

Changed reviewer from Sébastien Labbé, Nicolas M. Thiéry to Sébastien Labbé

@seblabbe
Copy link
Contributor

Changed author from Thierry Monteil to Thierry Monteil, Nicolas M. Thiéry

@vbraun
Copy link
Member

vbraun commented Nov 27, 2016

Changed branch from u/nthiery/package_rst-to-ipynb to d5d5680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants