Skip to content
This repository has been archived by the owner on May 2, 2020. It is now read-only.

Make installable with setup.py #33

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

talwrii
Copy link

@talwrii talwrii commented May 10, 2017

There are various advantages to a project being setuptools-compatible:

  • One-line installation with python setup.py install git+https://github.com/augustt198/latex2sympy#egg=latex2sympy (no additional steps). This encourages adoption
  • The ability of other packages to depend upon this package.

I have made this library python3 compatible. This is because (anltr in their wisdom) have made the newest version of their library incompatible with python2. (Edit: we only depend upon antlr4-python[23]-runtime not antlr-ast)

Caveats

Evil magic to build code at install time

(Edit: This library uses a generated parser that is stored in latex2sympy/gen/).

There's a bit of evil magic here. Python's support for code generation during a build step leave a little to be desired. I would class the approach use as a hack.

An alternative approach is to commit the generated grammar into the source tree, possibly adding a test to ensure that it hasn't been updated. This has the advantage of removing the dependency upon ANTLR for installation and converting evil magic into tests. This is likely there approach I would use if this were my own project. How happy you are with this depends upon your DRY sensibilities.

This changes the package structure.

This changes the package structure. This could break pre-existing code. We could work around this by installing individual modules rather than a package. I think there's a strong argument for not having a module in the global namespace called process_latex. How happy you are with this approach depends upon how happy you .are to inconvenience existing users.

@talwrii
Copy link
Author

talwrii commented May 10, 2017

antlr-ast no longer supports python3 (datacamp/antlr-ast#7)

@talwrii
Copy link
Author

talwrii commented May 10, 2017

Whoops. I was wrong about the dependency issues. This package depends upon antlr4-python[23]-runtime (edit: not antlr-ast). I've updated setup.py to depend upon the appropriate version.

@bollwyvl
Copy link

bollwyvl commented Dec 4, 2017

@augustt198, really love the work!

@talwrii This is a great start towards packaging! I'll sprinkle some comments throughout.

Been meaning to post here for a while! I don't know much about antlr, latex, or really sympy for that matter, but I've worked with folks that would really like to have tools like this in their work and teaching toolbelt.

I built up the various packages in conda, a natural fit for a polyglot build chain like this. I followed the conda-forge approach, and arrived at some locally-testable packages which appear to work just fine! Here's a shot of some roundtrips in Jupyterlab, next to inline and block representations:

screen shot 2017-12-04 at 12 01 40 am

Here are the recipes:

https://github.com/bollwyvl/staged-recipes/tree/latex2sympy/recipes

You can get them all built with:

conda install conda-build
conda build -c conda-forge recipes/latex2sympy  # should build its deps!

The latex2sympy recipe is based off this PR, so I haven't submitted those yet... but could start the process for the various bits of the chain. As suggested elsewhere, the antlr dependency isn't required at runtime, but it's nice to have a cross-platform plan for getting it built. If we can get this-a-here PR landed, potentially with a few fixes, we could have packages conda-installable in no time, as well as pretty streamlined CI.

def compile_grammar():
here = os.path.dirname(__file__) or '.'
package_dir = os.path.join(here, 'latex2sympy')
subprocess.check_output(['antlr4', 'PS.g4', '-o', 'gen'], cwd=package_dir)
Copy link

Choose a reason for hiding this comment

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

Not sure what a "normal" install of this would be like, but I ended up needing an antlr.cmd wrapper, so I had to do some platform checking.

Copy link
Author

@talwrii talwrii Dec 4, 2017

Choose a reason for hiding this comment

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

It's good having a Windows user to test this, you have my sympathy :). Seems like a good change.

How hard is it to install antlr on windows? I don't really understand the windows development environment. If antlr is hard to install and not installed by default by the kind of people who would use this package then requiring people to install antlr could prevent adoption.

On debian antlr looks like this: https://packages.debian.org/stretch/all/antlr4/filelist

Conda on linux calls the executable antlr (rather than antlr4)

https://anaconda.org/conda-forge/antlr/files

$ tar --list -f ~/Downloads/antlr-2.7.7-5.tar.bz2  | grep  bin  
bin/antlr-config
bin/antlr

I'm not sure whether the windows conda antlr package even works :/

How is antlr installed on your machine?

Copy link

Choose a reason for hiding this comment

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

How hard is it to install antlr on windows? How is antlr installed on your machine?

Well, I built antlr4 as a conda package on osx/linux/win... and by "built", i mean dumped the .jar in place and made a wrapper script, rather than diving into maven witchcraft. Usually, this is acceptable for java-based commands... can't speak to anyone using conda as a java package manager. I'm not really a windows user (just used a VM), but The Conda Way is to, whenever possible, take care of that community, as people do Get Work Done on windows.

Conda on linux calls the executable

The linked version on conda-forge (2.7.7) is likely different enough to require a new package anyway. I didn't even test it, since the docs said antlr4.

requiring people to install antlr could prevent adoption.

Anyhoo... in testing, once the the packages are built, they just install, no problem.

But the bigger story is that antlr4 isn't even required at runtime of latex2sympy, just the runtimes (also built as conda packages). This means it doesn't strictly even need to be in conda, and indeed, include_package_data works with wheels as well (though I haven't tested that route). Instead of checking the gen folder in, just distributing it in your package is just fine, shifting the burden to release time rather than every-user-install time, which is usually a really good trade.

setup.py Outdated
author='august.codes',
author_email='augustt198@gmail.com',
description='Parse latex markup into sympy: suitable for programmatic modifcation',
license='GPLv3',
Copy link

Choose a reason for hiding this comment

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

Whoops! Looks like it's MIT below, which should have been changed here!

setup.py Outdated
author_email='augustt198@gmail.com',
description='Parse latex markup into sympy: suitable for programmatic modifcation',
license='GPLv3',
keywords='MIT',
Copy link

Choose a reason for hiding this comment

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

Could definitely use some more stuff, I suppose...

setup.py Outdated
description='Parse latex markup into sympy: suitable for programmatic modifcation',
license='GPLv3',
keywords='MIT',
url='',
Copy link

Choose a reason for hiding this comment

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

Seems like the main repo URL would be reasonable.

setup.py Outdated
license='GPLv3',
keywords='MIT',
url='',
packages=['latex2sympy'],
Copy link

Choose a reason for hiding this comment

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

i just like to use setuptools.find_packages

cmdclass=dict(
install=AntlrInstallCommand,
develop=AntlrDevelopCommand),
test_suite='nose.collector'
Copy link

Choose a reason for hiding this comment

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

Could just go to pytest...

setup.py Outdated

setuptools.setup(
name='latex2sympy',
version=0.1,
Copy link

Choose a reason for hiding this comment

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

Should be a string, and should ideally appear in exactly one place (i.e. latex2sympy/_version.py) and be read here.

Copy link
Author

Choose a reason for hiding this comment

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

Well, it already appears in one place: setup.py. But adding a _version.py (or perhaps __version__ in the package __init__.py and pulling this in would make a version number accessible from code. I can see the value of this when trying to work out exactly what version of this package is running. On the other hand, there is a price in terms of discoverability and I don't imagine this package particularly changing a great deal.

install=AntlrInstallCommand,
develop=AntlrDevelopCommand),
test_suite='nose.collector'
)
Copy link

Choose a reason for hiding this comment

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

In addition, I added include_package_data as well as a MANIFEST.in to capture all the extra files.

@talwrii talwrii force-pushed the talwrii--setupdotpy--2017-05-10 branch 2 times, most recently from a7e20d3 to f2fbb52 Compare December 4, 2017 16:00
Tal Wrii added 9 commits December 4, 2017 17:03
import * was causing nose to run sympy tests
This includes magic to generate antlr files when installed.

This works with `setup.py develop`, `setup.py install` and `pip`.
It does not, however, work with tox. (http://stackoverflow.com/questions/43897581/python-packaging-generate-a-python-file-at-installation-time-have-this-work-wi)
In light of simpler installation mechanism and new package structure.
Include instructions on building grammar in develop instructions.
We do not want to depend upon antlr-ast rather
the verbosely named antlr4-python3-runtime.

This has different versions depending upon
which version of python we are using.
@talwrii talwrii force-pushed the talwrii--setupdotpy--2017-05-10 branch from f2fbb52 to f3d3dc6 Compare December 4, 2017 16:05

setuptools.setup(
name='latex2sympy',
version="0.1.0",
Copy link

Choose a reason for hiding this comment

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

Ooops, sorry about the quotes!

@bollwyvl
Copy link

bollwyvl commented Dec 4, 2017

I made this binder so you can see how stuff is installed:

https://mybinder.org/v2/gh/bollwyvl/latex2sympy-demo/master

protip: while this works in classic notebook, you can go to /lab to see it in jupyterlab

@rbistolfi
Copy link

Hi guys!

What do you think about just committing the parser into the git tree? It seems to be a lot simpler than Including antlr in the build process. People concerned with outdated parser could just rebuild it. Another possibility would be to rebuild the parser from a CI system.

I am interested in using latex2sympy for evaluation of math questions in the context of online education. We already use Sympy for processing client side generated MathJax.

@talwrii
Copy link
Author

talwrii commented Dec 29, 2017

What do you think about just committing the parser into the git tree?

To quote myself in the initial comment:

An alternative approach is to commit the generated grammar into the source tree,
possibly adding a test to ensure that it hasn't been updated. This has the advantage of > removing the dependency upon ANTLR for installation and converting evil magic into > tests. This is likely there approach I would use if this were my own project. How happy > you are with this depends upon your DRY sensibilities.

Yes, antlr seems like such a pain to install on some systems (particularly windows) that this would appear to be a good idea. On the the other hand, people can get cranky if you try to introduce redundancy into their pet projects!

I think no one commenting on this issue considers themselves to be "responsible" for the project. I think the initial developer might be busy with other things. Given this, I think a friendly fork with a name change might be in order.

I would suggest:

  • Emailing / dming / redditing the author asking for permission (Yes this is licensed under MIT, but there is such a thing as politeness)
  • Forking with a new name and committing the anltr files
  • Find a group of data scientists / scientists to be "maintainers". This probably isn't me as I don't use this tool on a regular basis.
  • Release (publicise)

Maintainers is a big word. Basically we just need someone to accept pull requests.
I feel as if a new name is necessary to indicate new maintainership, though one imagines if the author could be convinced to link to this new project that might work as well.

My understanding is/was that the original author is a high school student... so I imagine he might have moved on to new things. No judgement here! I just think high school students might be inclined to change their interests more rapidly than those whose interests are influenced by their income! An indication to the contrary on the behalf of the original author is more than welcome.

@bollwyvl
Copy link

bollwyvl commented Dec 30, 2017 via email

@talwrii
Copy link
Author

talwrii commented Dec 30, 2017

codebase into sympy proper:

Good idea: make sympy responsible for accepting pull requests :)

@bollwyvl
Copy link

bollwyvl commented Dec 31, 2017 via email

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

Successfully merging this pull request may close these issues.

3 participants