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

Questions regarding the dependencies for the conda package #43

Open
ocefpaf opened this issue Apr 7, 2016 · 16 comments
Open

Questions regarding the dependencies for the conda package #43

ocefpaf opened this issue Apr 7, 2016 · 16 comments

Comments

@ocefpaf
Copy link
Member

ocefpaf commented Apr 7, 2016

@emiliom the current requirement.txt has

pyodbc
pymysql
six
sqlalchemy
geoalchemy
#https://github.com/ODM2/geoalchemy/archive/v0.7.3.tar.gz
shapely
dateutils
pandas
#psycopg2  # Commented out because I could not pip install it.
#matplotlib
#sqlalchemy-migrate

The forked geoalchemy is easy to solve in a conda package, but I have a few concerns regarding other packages.

  • dateutils is outdated and unavailable in Python > 2.7. Maybe it is worth revisiting the code that uses dateutils and adapt it to a modern library;
  • psycopg2 is commented out and I don't know if it is an optional or mandatory dependency. Also, it is unavailable on Windows. (But it might be available soon. See initial commit of adding psycopg2 conda-forge/staged-recipes#101);
  • matplotlib is also commented out. I am guessing that is an optional dependency;
  • sqlalchemy-migrate same as above. There is no conda package for this one. We will need to add it here.

I usually recommend to add the optional dependencies when packaging with conda. What do you want to do here?

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

Thanks, @ocefpaf! I'm pinging @sreeder, the main odm2api developer/maintainer, so she can chime in.

  • dateutils: Completely agree. Let's identify where and how it's used and replace it with something that's widely used and maintained. @sreeder, I can tackle this if you'd like; by tomorrow or Monday.
  • psycopg2: I didn't know it was unavailable in Windows. I used to use it on Windows up to 3 years ago. It looks all you conda-forge gurus are actively working on this, so hopefully it'll be fixed soon?
  • matplotlib: @sreeder, do you know how it's used, and if it's truly needed as a requirement? If so, can it be replaced with something else? Just to keep requirements to bare minimum.
  • sqlalchemy-migrate: No idea about this. @sreeder?

Regarding geoalchemy, we forked this old and deprecated package for our own ODM2 needs. Our fork incorporates a small but important update. @ocefpaf, do you think it'd be helpful if we renamed it on the conda package as something unambiguous, like geoalchemy-odm2? Just so others understand more clearly that it's not exactly the same as https://github.com/geoalchemy/geoalchemy

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

Ok, re-reading the questions and my answers ... regarding matplotlib and sqlalchemy-migrate, I see now that they were already commented out months ago, by us! So I assume it's safe to ignore them (and remove them from requirements.txt, as they're no longer dependencies; @sreeder, can you confirm?

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 7, 2016

Just so others understand more clearly that it's not exactly the same as https://github.com/geoalchemy/geoalchemy

If that change is not going upstream yes. I do recommend you to renamed it.

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

Just so others understand more clearly that it's not exactly the same as https://github.com/geoalchemy/geoalchemy

If that change is not going upstream yes. I do recommend you to renamed it.

It's not. https://github.com/geoalchemy/geoalchemy ("geoalchemy1") was last updated 3 years ago and is no longer maintained. geoalchemy2 is its replacement, but it focuses only on PostgreSQL, not multiple RDBMS as geoalchemy1 did. We have no intention of taking up that burden; our use and tweak/fix to geoalchemy1 was only a stopgap for our own benefit to allow us to move forward with odm2api. We're happy to make it available to others via github and anaconda, though.

How/where should we rename it? Should we rename the github repo itself? Or just the conda package?

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 7, 2016

How/where should we rename it? Should we rename the github repo itself? Or just the conda package?

That is up to you. I would rename the repository and the conda package. This is a fork that separated from the original tree. Note that renaming everything comes with an extra maintenance burden. However, if ODM2 is the only group using this fork, then renaming just the conda package is fine.

@sreeder
Copy link
Contributor

sreeder commented Apr 7, 2016

matplotlib is only being used in the sample file to show a user that they can plot the time series data. so it is not required by the api itself.
I have no idea what sqlalchemy-migrate is used for so I have no issues getting rid of it.
I think dateutils is a requirement of one of our required packages. I don't think there is anywhere that we are using it directly
I didn't know that psycopg2 wasn't working on windows. I have been using it for testing on my windows machine. but I could be using an outdated version. I do know that I had to have postgres installed before I could successfully get psycopg2 to install.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 7, 2016

I didn't know that psycopg2 wasn't working on windows.

To be honest, if you already have postgresql installed, than you can easily install psycopg2. (Or you can use one of the wheels out there. I believe it is pip installable now.)

I meant that a conda package for psycopg2 (that installs its own postgresql) is not ready yet.

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

re: psycopg2, thanks for the clarification.

@sreeder, in the long run, we should work towards making each of the RDBMS packages optional (except for SQLite, but that's a non-issue); users of odm2api who are never going to use, say, postgresql and mysql, shouldn't have to install those dependencies, even if they're easy to install via conda. But this is a separate issue, not a near-term priority; I've created a new issue (#44) for future reference.

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

matplotlib is only being used in the sample file to show a user that they can plot the time series data. so it is not required by the api itself. I have no idea what sqlalchemy-migrate is used for so I have no issues getting rid of it.

Thanks. Let's fully get rid of them from requirements.txt.

I think dateutils is a requirement of one of our required packages. I don't think there is anywhere that we are using it directly

That rings a bell. It might be in geoalchemy. I'll look into it later today and report back.

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

That is up to you. I would rename the repository and the conda package. This is a fork that separated from the original tree. Note that renaming everything comes with an extra maintenance burden. However, if ODM2 is the only group using this fork, then renaming just the conda package is fine.

Let's rename the conda package only (to geoalchemy-odm2), at least for now. We may end up deciding to rename the github repo as well, but that'll be more effort so we'll need more time.

@emiliom
Copy link
Member

emiliom commented Apr 7, 2016

@sreeder, I'm not finding dateutils usage anywhere. I've searched through the code repositories for odm2api, geoalchemy, shapely, pyodbc and pymysql, and didn't find it. I seriously doubt it'd be required by sqlalchemy, pandas or six.

It looks like it's a remnant from something else done previously and it's no longer used by odm2api or its dependencies. If so, cool! @ocefpaf, unless @sreeder, wants to take a closer look, please get rid of dateutils from requirements.txt and requirements_tests.txt

@emiliom
Copy link
Member

emiliom commented Apr 8, 2016

@sreeder, should we go ahead with the conclusion that dateutils is not needed at all and should be removed from the requirements?

@valentinedwv
Copy link
Member

I think we can remove dateutils. It might have been a dependency in another package.

setup_clean has a good set of dependencies.
https://github.com/ODM2/ODM2PythonAPI/tree/setup_clean

@emiliom
Copy link
Member

emiliom commented Apr 8, 2016

@valentinedwv, thanks. We'll go with that.

@sreeder
Copy link
Contributor

sreeder commented Apr 8, 2016

@emiliom I say go for it. Could you take care of it for me? I am taking a personal day today, so I am not in the office. Thanks!

@emiliom
Copy link
Member

emiliom commented Apr 8, 2016

Thanks for the heads-up, @sreeder. Yup, we'll take care of it on the conda end. Looks like @valentinedwv has already taken care of it on the setup_clean branch he's working on for pypi packaging.

Based on Stephanie being out today, looks like Monday will be the best day to resolve all setup changes and align our pypi and conda packaging efforts. I'll start a new issue about this in a few minutes.

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

No branches or pull requests

4 participants