glue-viz / glue

Linked Data Visualizations Across Multiple Files
http://glueviz.org
Other
740 stars 153 forks source link

Add a check for FITS files mistaken as ASCII to ``astropy_table_read`` for Python 3.11 compatibility #2321

Closed dhomeier closed 2 years ago

dhomeier commented 2 years ago

Description

Trying now that Astropy is testing with 3.11.0rc2

dhomeier commented 2 years ago

Failure in data_factories.astropy_tabular_data('single_table.fits') – see astropy/astropy#13670. Maybe the Table.read(*args, format='ascii', **kwargs) attempt can be skipped now as the automatic format detection should long have been fixed.

dhomeier commented 2 years ago

Maybe the Table.read(*args, format='ascii', **kwargs) attempt can be skipped now as the automatic format detection should long have been fixed.

Somewhat unexpectedly just trying Table.read(guess=True) is still not detecting those ASCII formats widely enough, so I have put the extra check inside the existing wrapper. Also not so surprising, there is some resistance to have such a check basically override the specified format='ascii' within astropy.table, so this may need to stay in here.

dhomeier commented 2 years ago

To sum up some of the discussion in https://github.com/astropy/astropy/issues/13670#issuecomment-1249480276, a somewhat cleaner solution would indeed be to first try the read with automatic format recognition (which would correctly identify the FITS file) and then in case of failure make a second attempt with format='ascii'. That approach would also work, but change some results from the present behaviour, e.g. the test csv data "#a,b\n1,2" is parsed as standard header (interpreting the column names as ['#a', 'b']) when using full automatic detection, but as ascii.commented_header (column names ['a', 'b']) when reading with format='ascii'. So the tests would need to be adapted, but more importantly the default behaviour would change from what some users might have become used to.

dhomeier commented 2 years ago

Matplotib 3.6.x errorbar failures seem to be indeed specific to Python 3.10; passing at least with 3.11rc2.

astrofrog commented 2 years ago

@dhomeier - could you rebase this? The errorbar error should be gone now.

dhomeier commented 2 years ago

Yes, and the remaining tests should mostly be trimmed back to those from #2334 – I'd probably add py311-pyqt64 and py311-pyside64, (or :63, depending on the status of #2318).

dhomeier commented 2 years ago

macos: py311 probably needs brew hdf5 as well.

dhomeier commented 2 years ago

I suspect macos: py311 yet needs something like HDF5_DIR=/usr/local/homebrew/opt/hdf5 (or wherever its default install path is, but why is homebrew not setting this up properly on its own?!?) added to $GITHUB_ENV, but suppose this is still only a feature under discussion in OpenAstronomy/github-actions-workflows#56.

dhomeier commented 2 years ago

Yes, having a default version to use in ci_workflows would be good. The tests can actually run without h5py, but to properly set this up, I should perhaps figure out how to have h5py as a test-dep only for OS != macOS platform_system=='Linux' – I seem to have missed that it fails to build on Windows as well! 😄

dhomeier commented 2 years ago

No 3.11.0 for macOS yet. The Windows failures might rather be pandas errors. And this

File "/home/runner/work/glue/glue/.tox/py38-test-pyqt514-all/lib/python3.8/site-packages/pytestqt/logging.py", line 5, in from py._code.code import TerminalRepr, ReprFileLocation ModuleNotFoundError: No module named 'py._code'; 'py' is not a package

seems just randomly broken today!

pllim commented 2 years ago

new pytest was released too 😬

dhomeier commented 2 years ago

So that's the nose deprecation in Astropy? Had been wondering if they'd deprecate that between 7.1 and 7.2.

dhomeier commented 2 years ago

But generally, if h5py is removed from install_requires here and just kept as test dep, that will not affect your test-deps downstream, right?

pllim commented 2 years ago

if h5py is removed from install_requires here and just kept as test dep, that will not affect your test-deps downstream, right?

Correct.

dhomeier commented 2 years ago

So the py dependency was actually missing from pytest-qt==4.1.0, which is strangely pulled in by some runs, but not others. Was just removed in pytest-qt 4.2.0, so hopefully that should sort itself out.

dhomeier commented 2 years ago

@pllim would this be acceptable for resolution of the h5py dependency issue in lieu of #2341?

pllim commented 2 years ago

Yes, if you deem it sufficient, you can close #2341 without merge and go with this PR. Thanks!

dhomeier commented 2 years ago

macOS with 3.11 now works (without h5py); pyside63 seems a bit unstable...