search

MillionConcepts / pdr

[P]lanetary [D]ata [R]eader - A single function to read all Planetary Data System (PDS) data into Python
Other
62 stars 7 forks source link

make python-Levenshtein optional to allow jupyter-lite compatibility #67

Closed AndrewAnnex closed 2 months ago

AndrewAnnex commented 2 months ago

as part of openjournals/joss-reviews#7256,

suggesting that the python-Levenshtein dependency be made optional to allow compatibility with jupyterlite. I checked the other explicit dependencies listed in the pyproject.toml and they all seemed available. Looking through the repo it also looks like that dependency is only used for one function in the api and it may not be that well used/known.

Sierra-MC commented 2 months ago

Did a bit of looking into JupyterLite and while it is a cool project it is still unofficial. We also state in our documentation that we only officially support installation into a conda environment which JupyterLite is not compatible with. While there is only the one function in the API that uses it, that function is used heavily is several processing pipelines so we'd rather not leave out the dependency in our recipe.

AndrewAnnex commented 2 months ago

@Sierra-MC what do you mean by "unofficial" here? As pdr doesn't implement any c level code, by making the dependency optional you instantly gain compatibility with pyodide with no other work needed beyond the less than 5 lines of code changes necessary (it may literally be 2). Also, you do support setuptools/pip installs via the pyproject.toml and you distribute PDR on PyPI so I don't think it is true to say conda is the only officially supported way to install PDR. I don't think this issue is a blocker for the JOSS review, it is just a quick suggestion that I suspect will make pdr even more accessible

cmillion commented 2 months ago

We provide a pip install option as a convenience. We only officially support install via conda-forge in a pdr conda environment.

Sierra-MC commented 2 months ago

@AndrewAnnex I wasn't familiar with JupyterLite so I went to their webpage. The section on status states "Although JupyterLite is currently being developed by core Jupyter developers, the project is still unofficial."

So by "unofficial" I'm referring to that statement specifically, not like, any particular judgement on it. Like I said this is the first I'm hearing of the project so far be it from me to judge their status. I looked at their issues and they do have an open issues to be able to check if packages are compatible (https://github.com/jupyterlite/jupyterlite/issues/967) and to better document the limitations (https://github.com/jupyterlite/jupyterlite/issues/199). Without familiarity I wasn't entirely positive what made Levenshtein incompatible (though you've clarified here it is the C code which is what I suspected from the bit of poking I did do). The team discussed the possibility of changing the dependency to optional in the project.toml, however this would make it auto-install for conda users and not for pip users. Which is a little confusing behavior and potentially difficult to diagnose.

I'm not really sure how big of a market the pyodide community represents so it's difficult to assess the degree of accessibility that this opens and weigh that against the possible confusion the different installation behavior would cause.

Sierra-MC commented 2 months ago

This has continued to eat at me the past couple of days and after further discussion we have decided to make the change you suggested @AndrewAnnex . JupyterLite may end up being what we use to replace binder, but even if not you've made good points that we can allow further compatibility with pyodide. This was changed in this commit on develop so it will be compatible in our next release. Thank you for the suggestion and for bringing this package to our attention.

We will also be removing the dependency from the conda recipe for consistency. (clarifying for posterity)