Refactor JPLSpec/CDMS to bring common code up a level #3456

keflavich · 2025-11-07T21:59:04Z

I'm not as sure as I was at the start of this project that there is enough in common, but maybe.

This relies on other open PRs, #3302 and #3455 should be merged before this one, and then this one needs squashing.

keflavich · 2025-11-08T01:05:40Z

My hacking has hit an awkward position. I wanted to implement a fallback-to-getmolecule approach, but this made me realize that the async-to-sync method prevents returning non-response results and prevents passing arguments directly to _parse_response. Both of these make the workaround a lot more awkward.

There are a couple other approaches I could take, but I really want this fallback because of the unreliability of JPLspec and the hinted-at fragility of other services.

bsipocz · 2025-11-08T17:02:08Z

What about ditching async to sync? I have my doubts it ever worked in a real async way

keflavich · 2025-11-08T17:10:53Z

yeah, I can get onboard with that. It's a big refactor, but not that hard.

That said, I'd need to feel quite inspired / have little on my plate to want to take that on myself. I think it is not the tallest nail right now

keflavich · 2025-11-08T17:11:24Z

(also most of the problems I'm experiencing here are because the SPCAT format is extraordinarily arcane and has dozens of undocumented edge cases....)

bsipocz · 2025-11-08T18:09:46Z

Of, I don't mean to refactor the machinery, but feel free to cut the usage of it. We don't use it in the vo backed modules anyway, so it's more like a slow cleanup rather than one big push.

keflavich · 2025-11-08T18:15:01Z

right, good point. OK, I'll think about that - once I get out of qn-parsing hell

codecov · 2025-11-09T13:32:08Z

Codecov Report

❌ Patch coverage is 79.61783% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.68%. Comparing base (4c01218) to head (03747d5).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/linelists/jplspec/core.py	73.39%	29 Missing ⚠️
astroquery/linelists/cdms/core.py	94.11%	2 Missing ⚠️
astroquery/linelists/core.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3456      +/-   ##
==========================================
+ Coverage   71.48%   71.68%   +0.20%     
==========================================
  Files         234      236       +2     
  Lines       20096    20226     +130     
==========================================
+ Hits        14365    14499     +134     
+ Misses       5731     5727       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

keflavich · 2025-11-09T21:07:11Z

I had an LLM write some of the tests to increase coverage. I reviewed those tests, and they look right, but I'm not certain. I can say that they revealed genuine bugs that I had to fix, so that was good.

keflavich · 2025-11-09T21:16:02Z

#3302 and #3455 should be merged before this one, and then this one needs squashing.

bsipocz · 2025-11-12T21:45:08Z

@keflavich - I believe you pushed to the wrong branch

keflavich · 2025-11-12T23:15:20Z

oh ffs.... you're right.

keflavich · 2025-11-12T23:51:38Z

I'll have to rebase this one after merging #3455 anyway.

bsipocz · 2025-11-19T13:53:05Z

As both parent PRs are merged, can you rebase this and squash out iterations as necessary here?

keflavich · 2025-11-19T13:54:20Z

Yes, let me handle that - it will be complex and I will need to remove some things

keflavich · 2025-11-29T12:35:46Z

This should be ready for review, but I'm less confident than I'd like to be about the rebase. vscode created ridiculous, incomprehensible merge failures in the changelog, so I basically rewrote it from scratch. The rest... most of the conflicts were just import or whitespace errors, so I'll check back through these.

keflavich · 2025-11-29T12:38:46Z

I verified that the doctests whitespace are correct with these commands, which resulted in no diff (to my shock!):

pytest docs/linelists/jplspec/jplspec.rst --doctest-plus-generate-diff=overwrite -R 
pytest docs/linelists/cdms/cdms.rst --doctest-plus-generate-diff=overwrite -R

keflavich · 2025-12-01T23:46:20Z

Tests are passing. The OSX failure was unrelated, caused by something in ESA. I'm re-running, but I recommend ignoring that fail even if it re-fails.

The commit messages below come from individual commits that were squashed into one. A lot of other messages were redundant and manually edited out: factor pseudo-common code into linelists from cdms/jplspec refactor jplspec to not use async machinery refactor to use absolute imports

bsipocz

Here are some quick review comments.

Big picture: I think we can follow suite of the other modules and remove the not really async methods, and if available have async_job to control the logic to do server side async if supported.

There are some missing docs.

And, it may make sense to have a baseclass for the linelists, that would ensure some API consistency as we go ahead and add more of these.

bsipocz · 2025-12-09T19:03:48Z

CHANGES.rst

+linelists
+^^^^^^^^^
+
+- General tools for both CDMS/JPL moved to linelists.core [#3456]


not sure we need this line as users are not expected to interact with core.

fair enough, but the changelog is also a little bit for devs, no? I'm fine removing this

ok, then we can keep it. I may still need to reformat it before release to be more specific; and thus useful for the devs :)

astroquery/linelists/__init__.py

bsipocz · 2025-12-09T19:05:56Z

astroquery/linelists/cdms/core.py

 # import configurable items declared in __init__.py
 from astroquery.linelists.cdms import conf
 from astroquery.exceptions import InvalidQueryError, EmptyResponseError
+from ..core import parse_letternumber


I don't think we need to use relative imports, stick with absolute ones.

...right. Not sure why I did this.

astroquery/linelists/cdms/core.py

bsipocz · 2025-12-09T19:25:02Z

CHANGES.rst


 - Fix bug in queries for interstellar objects with `MPC.get_observations` and enable queries for "dead" comets [#3474]

+linelists


if I see correctly, JPLSpec.get_molecule() is a brand new thing. Please mention it above in the linelist.jplspec section.

bsipocz · 2025-12-09T19:25:42Z

docs/linelists/jplspec/jplspec.rst

Please add a section/example about the new get_molecule()

this is on my to-do list for both jplspec and cdms

bsipocz · 2025-12-09T19:26:54Z

astroquery/linelists/jplspec/core.py


        return result

+    def get_molecule(self, molecule_id, *, cache=True):


This needs to be added to the changelog as well as to the narrative docs

bsipocz · 2025-12-09T19:31:24Z

astroquery/linelists/jplspec/core.py

    return os.path.join(data_dir, filename)


-@async_to_sync


Can we remove the async methods as they are not really server async? Or if there is server async available, follow the new API and use the async_job kwargs instead?

I'd rather not do that in this PR

bsipocz · 2025-12-09T19:33:31Z

astroquery/linelists/jplspec/core.py



-@async_to_sync
 class JPLSpecClass(BaseQuery):


It goes beyond this PR, but maybe you want to open an issue about coming up a linelist relevant baseclass? E.g it could behave as a template, e.g. we always have to have a query_lines method, etc.

(but also, this may be an overkill)

maybe, not sure it's needed - so, yeah, beyond this PR

keflavich marked this pull request as ready for review November 9, 2025 21:05

keflavich mentioned this pull request Nov 12, 2025

Move JPLSpec to linelists/ #3455

Merged

bsipocz added jplspec cdms labels Nov 19, 2025

bsipocz added this to the 0.4.12 milestone Nov 19, 2025

keflavich mentioned this pull request Nov 29, 2025

JPLSpec: Download everything as a workaround for nonfunctional queroes #3396

Closed

keflavich force-pushed the jplspec_getmolecule branch 6 times, most recently from 5b8ed10 to 203a0a5 Compare November 29, 2025 12:33

remove unneeded imports [should fix all codestyle failures]

03747d5

keflavich force-pushed the jplspec_getmolecule branch from 4843bf0 to 03747d5 Compare December 9, 2025 13:51

bsipocz reviewed Dec 9, 2025

View reviewed changes

address review comments

37fe09c


		- Fix bug in queries for interstellar objects with `MPC.get_observations` and enable queries for "dead" comets [#3474]

		linelists


		return result

		def get_molecule(self, molecule_id, *, cache=True):

Uh oh!

Refactor JPLSpec/CDMS to bring common code up a level #3456

Are you sure you want to change the base?

Refactor JPLSpec/CDMS to bring common code up a level #3456

Uh oh!

Conversation

keflavich commented Nov 7, 2025 • edited by bsipocz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keflavich commented Nov 8, 2025

Uh oh!

bsipocz commented Nov 8, 2025

Uh oh!

keflavich commented Nov 8, 2025

Uh oh!

keflavich commented Nov 8, 2025

Uh oh!

bsipocz commented Nov 8, 2025

Uh oh!

keflavich commented Nov 8, 2025

Uh oh!

codecov bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

keflavich commented Nov 9, 2025

Uh oh!

keflavich commented Nov 9, 2025

Uh oh!

bsipocz commented Nov 12, 2025

Uh oh!

keflavich commented Nov 12, 2025

Uh oh!

keflavich commented Nov 12, 2025

Uh oh!

bsipocz commented Nov 19, 2025

Uh oh!

keflavich commented Nov 19, 2025

Uh oh!

keflavich commented Nov 29, 2025

Uh oh!

keflavich commented Nov 29, 2025

Uh oh!

keflavich commented Dec 1, 2025

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

keflavich commented Nov 7, 2025 •

edited by bsipocz

Loading

codecov bot commented Nov 9, 2025 •

edited

Loading