search

tlubitz / SBtab

SBtab - Table format for Systems Biology:
www.sbtab.net
MIT License
6 stars 8 forks source link

What is the reason for the openpyxl <= 2.6.4 requirement? #119

Closed danolson1 closed 9 months ago

danolson1 commented 1 year ago

What is the reason for the openpyxl <= 2.6.4 requirement? Pandas 1.5.3 requires openpyxl > 3.0.7, and it is convenient to be able to use SBtab and Pandas together.

-Dan

eladnoor commented 10 months ago

I don't know the reason for the requirement, but I don't have a problem installing Pandas 2.1.3 alongside openpyxl 2.6.4.

danolson1 commented 10 months ago

Yeah, I was able to install Pandas 2.1.2, but I had to make a local copy of Sbtab and comment out the openpyxl requirement. I was thinking about trying to see if this interfered with any of the code tests, but they're all set up to run in Linux and I'm running Windows.

I believe the requirement is in this file https://github.com/tlubitz/SBtab/blob/master/pypi_installer/setup.cfg.

Is it possible the the setup.cfg file hasn't been updated recently and is not consistent with the requirements.txt file?

-Dan

danolson1 commented 10 months ago

The Pandas requirement for openpyxl may only be relevant if you're trying to use the read_excel() function in Pandas.

danolson1 commented 10 months ago

It looks like openpyxl 3.0.6 got rid of support for numpy.float https://foss.heptapod.net/openpyxl/openpyxl/-/issues/1568, and this causes some kind of error message when trying to read float values from *.xlsx documents. I don't remember the exact error message off the top of my head, but if it was important, I could probably recreate it.

eladnoor commented 10 months ago

Why don't we simply make the openpyxl dependency optional? I think we should encourage users to use CSV/TSV instead of XLSX anyway. Also, I think there are no tests at the moment for Excel I/O (which could have helped locate this bug earlier).

danolson1 commented 10 months ago

I think the simplest fix would be to update setup.cfg in the pypi_installer folder ("openpyxl<=2.6.4") to match the requirements.txt file in the root directory ("openpyxl>=2.5"). If the dependencies are supposed to be identical in both places, you might also check the python-libsbml requirement.

If the openpyxl dependency is not needed, I agree that making it optional is a good idea, particularly if there are no tests for it.

In my opinion, a key benefit of the SBtab format is its flexibility, so I don't think the SBtab library should be trying to push users toward one input format over another (CSV vs. XLSX, for example). Breaking openpyxl compatibility makes it difficult for non-expert users to use SBtab in a Python environment where they are also using XLSX files (even if the XLSX files aren't directly being used for SBtab input).

tlubitz commented 10 months ago

Hi guys, let me know if I can help. As far as I see it, the suggested fix (setting openpyxl requirement to >=2.5 in the setup.cfg) makes absolute sense. @eladnoor are you able to make that change?

eladnoor commented 10 months ago

I created a pull request for this #121. This fixes only the pypi_installer directory. I'm not sure why there are two copies of the code so I didn't touch the python folder.

eladnoor commented 9 months ago

@tlubitz - can you review and accept the PR? Then, we should update the PyPI version I guess.

tlubitz commented 9 months ago

I reviewed and accepted. Do you do the merge?

I can try to rebuild the PyPi version then (I always need to recall how this went down... been some time).

eladnoor commented 9 months ago

Okay, I just merged. I think you usually ran the twine upload command locally

tlubitz commented 9 months ago

Yep. Luckily Past Timo left some written notes for Today Timo in order to manage this without friction.

sbtab version 1.0.8 is online

https://pypi.org/project/sbtab/

eladnoor commented 9 months ago

Wow, that was fast! Thanks a lot!

tlubitz commented 9 months ago

Sure! I am in getting-everything-off-my-desk-for-the-holidays mood. Tomorrow's my last day of work this year, so everything needs to be done asap. :)

Have a nice end of the year, Elad, although Hanukkah is already over. I hope you guys are faring well, I'm thinking of you in these trying times.

eladnoor commented 9 months ago

Okay, so I guess I had perfect timing :) Happy holidays to you too, and thanks for the help and support.