search

qiime2 / q2-composition

BSD 3-Clause "New" or "Revised" License
5 stars 28 forks source link

Downgrade pandas to <0.23 instead of >=0.23 #58

Closed ElDeveloper closed 6 years ago

ElDeveloper commented 6 years ago

This allows for other packages (namely scikit-bio 0.5.3) to be compatible with current QIIME2-dev distribution.

In the long run we want to make our dependency on pandas more flexible i.e. not limited to <0.23 but that will require a fair amount of changes in scikit-bio. If this PR is merged, I'll open an issue (in this repo) to keep track of this and make sure we don't forget to re-allow for pandas >=0.23

Related to https://github.com/qiime2/q2-types/pull/191

mortonjt commented 6 years ago

Thanks for pushing this in! Should be good once the tests pass

mortonjt commented 6 years ago

grrrr ... looks like there are more pandas bugs popping up ...

mortonjt commented 6 years ago

🤔 strange -- its not throwing errors on my machine ...

HannesHolste commented 6 years ago

@mortonjt @ElDeveloper According to the travis CI log, it's setting up the qiime2 environment using the conda env file from the web: 

$ wget -q https://raw.githubusercontent.com/qiime2/environment-files/master/latest/staging/qiime2-latest-py35-linux-conda.yml
install.2
545.06s$ conda env create -q -n test-env --file qiime2-latest-py35-linux-conda.yml

That env file specifies pandas=0.23.4, which ought to be < 0.23. Hence why it works on your local machines but fails on Travis.

mortonjt commented 6 years ago

Nice catch @HannesHolste ! @ebolyen how hard would it be to downgrade the pandas requirement in the conda env file?

HannesHolste commented 6 years ago

@ebolyen by extension to Jamie's question: is it just a matter of updating the pandas version in q2-types ?

ElDeveloper commented 6 years ago

Apologies for coming in a bit late, but the solution is to either merge this PR and wait for a new environment file to be produced by busywork or to trigger that from a different repository.

thermokarst commented 6 years ago

Hey @HannesHolste & @mortonjt - as @ElDeveloper mentioned, it is a bit of a chicken-and-egg type of issue. Travis uses the latest development environment kicked out by busywork. So, when it comes to changes in deps like this, the new dep won't be available in the travis environment because the code hasn't been merged and subsequently percolated through busywork. Yikes!

Okay, back to @ElDeveloper's original post:

If this PR is merged, I'll open an issue

Yeah, generally speaking we don't want to force pins like this, since it has cross-cutting implications across all other q2-plugins. But, as @ElDeveloper mentioned, the goal is to temporarily get the latest skbio into q2, and we have a psuedo-game-plan for removing these upstream constraints (e.g. skbio's pandas requirements). Sorry, long-winded way to say: this makes sense to me, I am in favor of merging, since we know this should be temporary.

ElDeveloper commented 6 years ago

Cheers, thanks @thermokarst!

ElDeveloper commented 6 years ago

Thanks!

On (Aug-22-18|10:56), Evan Bolyen wrote:

Merged #58 into master.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/qiime2/q2-composition/pull/58#event-1803317267

mortonjt commented 6 years ago

Thanks!

On Wed, Aug 22, 2018, 10:58 AM Yoshiki Vázquez Baeza < notifications@github.com> wrote:

Thanks!

On (Aug-22-18|10:56), Evan Bolyen wrote:

Merged #58 into master.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/qiime2/q2-composition/pull/58#event-1803317267

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/qiime2/q2-composition/pull/58#issuecomment-415123146, or mute the thread https://github.com/notifications/unsubscribe-auth/AD_a3e-o4V1NY_B8VXmq_9D6XmlIY2LPks5uTZvVgaJpZM4WGx6r .