Updates to nasa_exoplanet_archive module to make it compatible with Archive 2.0

[rickynilsson]

The module has been updated to query the new PS and PSCOMPPARS tables using TAP service, and support for querying the old tables (EXOPLANETS, EXOMULTPARS, and COMPOSITEPARS) has been dropped (with error messages pointing the user to the new tables and documentation of recent changes to the tables and their accessibility). Tests have been updated and verified, and the module documentation has also been updated.

[pep8speaks]

Hello @rickynilsson! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-18 00:02:22 UTC

[bsipocz]

@rickynilsson - I don't see an obvious reason for the cancelled jobs (they will be cancelled when there one of the fails, but haven't seen any of them got to that stage yet), I've restarted the workflow.

[rickynilsson]

@rickynilsson - I don't see an obvious reason for the cancelled jobs (they will be cancelled when there one of the fails, but haven't seen any of them got to that stage yet), I've restarted the workflow.

Now codestyle check failed. Just fixed those issues and committed again.

[rickynilsson]

@bsipocz - the oldest dependencies (Python 3.6) tests should pass, as far as I can see. They run fine in my Python 3.6 environment, and https://exoplanetarchive.ipac.caltech.edu/cgi-bin/nstedAPI/nph-nstedAPI?table=aliastable&objname=HAT-P-11+b&format=csv responds as expected (as you can verify). Not sure how to troubleshoot this one.

[bsipocz]

The issue with that test (that will come up in all the other runs, too) that they are not marked with remote_data yet they reach out to the server. By default, we don't let any internet access happen.
I suppose if you run the tests locally you would see the same failure.

To fix it, decorate the test functions that reach out to the server with @pytest.mark.remote_data, see examples in modulename/tests/test_modulename_remote.py files.

[codecov]

Codecov Report

Merging #2067 (1fec091) into main (9696416) will decrease coverage by 0.25%.
The diff coverage is 59.73%.

@@            Coverage Diff             @@
##             main    #2067      +/-   ##
==========================================
- Coverage   66.81%   66.56%   -0.26%     
==========================================
  Files         407      407              
  Lines       27454    27647     +193     
==========================================
+ Hits        18344    18403      +59     
- Misses       9110     9244     +134     

Impacted Files	Coverage Δ
...rchive/tests/test_nasa_exoplanet_archive_remote.py	28.92% <22.82%> (-13.94%)	⬇️
astroquery/nasa_exoplanet_archive/core.py	69.45% <43.20%> (-25.08%)	⬇️
...lanet_archive/tests/test_nasa_exoplanet_archive.py	96.93% <96.09%> (-3.07%)	⬇️
astroquery/nasa_exoplanet_archive/__init__.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9696416...1fec091. Read the comment docs.

[rickynilsson]

@bsipocz - I decorated the remote tests and moved them over to the test_..._remote.py file where they belongs. I also added some more tests for increased code coverage. Verified test on my side: 43 passed, 19 warnings (these are mainly PytestUnknownMarkWarning: Unknown pytest.mark.remote_data for some reason). Codecov should hopefully be happier now.

[rickynilsson]

Hi @bsipocz, @keflavich, @ceb8,
Just tagging you to see if one of you can approve the workflow for the merge. This is currently one of the NASA Exoplanet Archive's high priority items, and we are hoping to announce the updated module to the exoplanet community soon (hopefully even in today's newsletter). Thanks!

[rickynilsson]

@bsipocz, @keflavich, @ceb8 - It appears checks were automatically cancelled for no obvious reasons (no failed check). This happened once before, but then went through when workflow was restarted.

[rickynilsson]

@bsipocz, @keflavich, @ceb8: CI / Python 3.8 with all dependencies with remote data (pull_request) got skipped and rest got cancelled again. Any idea what could be going on?

[bsipocz]

I'm not sure what's going on with these actions, I've just restarted the workflow but it got cancelled again.

[rickynilsson]

I'm not sure what's going on with these actions, I've just restarted the workflow but it got cancelled again.

Weird...
@bsipocz - Any chance I could get write access to re-run the actions myself, so that I don't have to keep bugging you? Or is there any workaround for merging this without passed checks?

[rickynilsson]

Thanks! I'll have to run Codecov locally and try to get the coverage up.

[rickynilsson]

@bsipocz - Coverage.py reports a total test coverage of 96%

---------- coverage: platform darwin, python 3.8.8-final-0 -----------
Name                                                                 Stmts   Miss  Cover
----------------------------------------------------------------------------------------
nasa_exoplanet_archive/__init__.py                                      10      0   100%
nasa_exoplanet_archive/core.py                                         225     14    94%
nasa_exoplanet_archive/tests/__init__.py                                 1      0   100%
nasa_exoplanet_archive/tests/setup_package.py                            3      3     0%
nasa_exoplanet_archive/tests/test_nasa_exoplanet_archive.py             56      0   100%
nasa_exoplanet_archive/tests/test_nasa_exoplanet_archive_remote.py     105      0   100%
----------------------------------------------------------------------------------------
TOTAL                                                                  400     17    96%

Not sure how Codecov calculates its numbers.

[rickynilsson]

@bsipocz - I wonder if the unexpectedly low diff coverage value comes from removed tests (most pertaining to the now unsupported tables, and deprecated methods, and some mock-data tests moved to remote tests). See https://docs.codecov.io/docs/unexpected-coverage-changes

I've added a few more tests to get the total coverage up to 97%. I'm happy to send you my coverage.xml generated from running:
pytest --cov=nasa_exoplanet_archive nasa_exoplanet_archive/tests/
coverage xml

Hoping to get over this Codecov hurdle somehow.

[bsipocz]

@bsipocz - Any chance I could get write access to re-run the actions myself, so that I don't have to keep bugging you? Or is there any workaround for merging this without passed checks?

Only the maintainers' team has write access to repos, not individuals. But anyways, write access is not needed, the CI is not run automatically due to github's being attached by crypto miners, and they turned off CI starting up for first-time contributors.

The known workarounds are:

• make a small PR that we can merge asap with a minor change, anywhere.
• or, add you to the large astropy contributors team.

I would love to do the latter (already tried it), but it's only the coordination committee is gatekeeping the access rights from the rest of the team to be able to add new people. So we need to fall back to the PR workaround. Any small dummy PR with a single commit would do, e.g. fix the alphabetical order for nasa_exoplanet_archive in docs/index.rst.

As for coverage: it sometimes behaves weirdly with bogus looking drop in coverage when the base of the PR and the latest run on master differs. I think simply there was a lot of cleanups in the recent 0.4.2. Also, you have restructured the tests, so it's possible that some parts of the code now covered only with remote-data tests, and the coverage run doesn't pick those up.
In summary: we look into the coverage status during review, but merge a PR even with a failing status if everything otherwise looks good (and the status can be easily explained with one of the scenarios above).

[rickynilsson]

@bsipocz - Any chance I could get write access to re-run the actions myself, so that I don't have to keep bugging you? Or is there any workaround for merging this without passed checks?

Only the maintainers' team has write access to repos, not individuals. But anyways, write access is not needed, the CI is not run automatically due to github's being attached by crypto miners, and they turned off CI starting up for first-time contributors.

The known workarounds are:

• make a small PR that we can merge asap with a minor change, anywhere.
• or, add you to the large astropy contributors team.

I would love to do the latter (already tried it), but it's only the coordination committee is gatekeeping the access rights from the rest of the team to be able to add new people. So we need to fall back to the PR workaround. Any small dummy PR with a single commit would do, e.g. fix the alphabetical order for nasa_exoplanet_archive in docs/index.rst.

As for coverage: it sometimes behaves weirdly with bogus looking drop in coverage when the base of the PR and the latest run on master differs. I think simply there was a lot of cleanups in the recent 0.4.2. Also, you have restructured the tests, so it's possible that some parts of the code now covered only with remote-data tests, and the coverage run doesn't pick those up.
In summary: we look into the coverage status during review, but merge a PR even with a failing status if everything otherwise looks good (and the status can be easily explained with one of the scenarios above).

Thanks @bsipocz! I made the suggested edit in docs/index.rst and submitted a PR, so hopefully I can run CI workflows soon. That will also be helpful over the next couple of months as we move more of our tables over to the TAP service.

[rickynilsson]

@bsipocz - Any idea when merge review might take place? Sorry for being impatient :-)

[rickynilsson]

@bsipocz, @keflavich, @ceb8, @dfm, @bmorris3 -
Hi ya'll. Just wanted to check in again about when merge review for these Archive 2.0 updates might happen. Cheers!

[bsipocz]

@rickynilsson - I'm sorry, I'm horribly swamped at the moment with the end of quarter and wrapping up my current day job. It may take a few weeks on my side to get enough time for a proper, thorough PR review.

[dfm]

I don't know enough about the astroquery requirements to do a full review, but I'm happy to take a stab at a review from a user perspective if that would be helpful, @bsipocz!

[dfm]

I'll try actually using this interface a little later, but it looks to me like the big remaining issue is that there appear to be no tests for the TAP interface that don't rely on remote data. Others can comment on whether or not that is okay, but I would certainly prefer to be able to test the interface using mocked responses.

[rickynilsson]

Thanks, @dfm ! I honestly couldn't figure out exactly what was going on in patch_get(request), which is why I left out mock tests for the TAP service. I didn't know they were necessary, and I suspect Codecov checks fail because of removed tests of deprecated tables (giving a low diff coverage). If you would want to help out setting up the mock tests for TAP that would be greatly appreciated. I'll clean up the other issues you mentioned.

[dfm]

@rickynilsson: Sure - it's a little opaque, I know! I'll send some demo tests to give you a sense of it.

[keflavich]

@andamian provides some good examples of mocking tests for TAP in https://github.com/astropy/astroquery/blob/main/astroquery/cadc/tests/test_cadctap.py

[dfm]

@rickynilsson: Looking through this more, I honestly think that a better interface design might be to have separate NasaExoplanetArchive and NasaExoplanetArchiveTAP classes. I worry that the current implementation is confusing in how it tries to abstract such different interfaces into one class. It might be better for users to be more explicit. Then the NasaExoplanetArchive interface can just correctly throw exceptions for the queries that are no longer supported instead of trying to shoehorn into the new interface. This would also make the testing (using the reference shared by @keflavich) and implementation simpler. What do you think about that?

[rickynilsson]

@bsipocz - totally understand that you are busy with work! Maybe if @dfm helps out with a quick review from a user perspective, we can merge fairly soon, and fix any bugs or lack of astroquery requirements (should be minor) later? We haven't touched any other modules, and thoroughly tested the restored functionality of our module. Note that the nasa_exoplanet_archive module in main is currently broken, and we are starting to get questions from users about it. Alternatively, we could direct users to our working branch on https://github.com/IPAC-SW/astroquery/tree/nasa_exoplanet_archive_2 , if that is okay with you.

[bsipocz]

I've rebased to get rid of the unrelated and duplicated commits, and did some minor cleanup. However, given that the PR was opened from an org account rather than a personal account I cannot force push back to the branch as a maintainer.
So, while for independent reasons I'll added to that org account, we may also think about how to get access for the other two maintainers (I'm happy to be the responsible maintainer for any IPAC stuff, so it probably won't come up as a blocker)

[bsipocz]

Ready to go once CI is happy.

[bsipocz]

I think this is a bit confusing in the warning message, but can be adjusted if there are any user complaints.

[bsipocz]

OK, so the only remaining issue is with the heisen failure of remote tests. 3 of them are failing if they are run after the mock tests, but they all pass when only the remotes are run.
I suggest to open a separate issue for it, and follow-up in a separate PR. I don't yet see whether it's a bug in the mock tests, or the code, or rather something related to how pytest is run. I kind of rules out it being cache related, as the tests pass even after they failed, if they are run on their own.

[bsipocz]

Thanks @rickynilsson!

Would you mind opening that follow-up issue, or prefer me to do it?

[rickynilsson]

Thank you @bsipocz! I can open the issue about order dependent test fails.

[bsipocz]

(you can let users know that installing the development version with e.g. python -m pip install git+https://github.com/astropy/astroquery.git would pick up this update).