eJWST prelaunch #2140

jespinosaar · 2021-08-23T07:00:17Z

Dear Astroquery team:

As James Webb Space Telescope (JWST) will be launched on November, we have developed a module to access future ESA JWST Archive using astroquery. I am creating this draft pull request in advance to prepare our module to be integrated in astroquery and inform you about the necessity of merging this pull request at an agreed date with you (before November, I will confirm you a specific date in a few weeks) and, if possible, agree in releasing a new version of astroquery.

This is still ongoing work, no major changes are expected, but you can have a look at this PR if you want and comment anything you think is useful.

Thanks in advance!

Kind regards,

Javier Espinosa

pep8speaks · 2021-08-23T07:00:27Z

Hello @jespinosaar! 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-11-26 18:38:01 UTC

keflavich · 2021-08-23T12:05:55Z

Thanks for the heads up. I certainly acknowledge the importance of a timely deployment for the JWST archive. Though I suspect we'll have a little while post-launch before there are any data to serve, no?

Is there any redundant code here with the other TAPPlus in astroquery? Can that be reused instead of copied?

jespinosaar · 2021-08-24T08:11:44Z

Hi @keflavich, thanks for your comments. Yes, we will have to wait some time before we have data to serve and, for this reason, we would like to have this PR merged before, to be ready when this happens.

I think I can refactor code that was copied from TAPPlus login window, as we needed a new one with an additional field. I will revise it together with new classes. Thanks!

jespinosaar · 2021-10-19T16:07:40Z

Hi @keflavich !

I have removed the repeated code. Now I can see I have errors in the oldest dependencies, which are not part of my module:

FAILED ../../.tox/py37-test-oldestdeps/lib/python3.7/site-packages/astroquery/simbad/tests/test_simbad.py::test_query_object[m1-None]
FAILED ../../.tox/py37-test-oldestdeps/lib/python3.7/site-packages/astroquery/simbad/tests/test_simbad.py::test_query_object[m [0-9]-True]

I have tried rebasing my branch, but it has not worked. How should I proceed? Thanks in advance!

keflavich · 2021-10-19T16:24:37Z

Good question, I'm not sure what this indicates. It does indeed seem to be unrelated. I wonder if a rebase is required, and your PR is being tested with some outdated code in other parts of astroquery? But I don't recall making any changes recently that would affect SIMBAD this way.

@bsipocz do you recall any change that would cause this failure? Maybe we were trying to account for a deprecated VOTable function and broke tests... but if so, why didn't CI catch the regression?

eerovaher · 2021-10-19T16:48:28Z

I don't know about the broken tests, but I did notice that this pull request would introduce quite a lot of Python2-related code to astroquery, which works against #1870. For example, six has been removed from astroquery and is no longer a direct requirement, but this pull request seems to rely on six quite a lot in astroquery/esa/jwst/token.py.

jespinosaar · 2021-10-20T15:33:15Z

I don't know about the broken tests, but I did notice that this pull request would introduce quite a lot of Python2-related code to astroquery, which works against #1870. For example, six has been removed from astroquery and is no longer a direct requirement, but this pull request seems to rely on six quite a lot in astroquery/esa/jwst/token.py.

astroquery/esa/jwst/token.py file has been removed, as, in the end, it was not necessary anymore. Which other classes do you think we should modify to remove Python2-related code?

Anyway, we still have the issue with SIMBAD...

eerovaher · 2021-10-20T15:47:23Z

astroquery/esa/jwst/tests/DummyTapHandler.py

+from astroquery.utils.tap.model.job import Job
+
+
+class DummyTapHandler(object):


In Python 3 classes inherit from object by default.

Suggested change

class DummyTapHandler(object):

class DummyTapHandler:

See also #1879

Object removed.

eerovaher · 2021-10-20T16:04:55Z

astroquery/esa/jwst/tests/DummyTapHandler.py

+        if parameters is None:
+            return len(self.__parameters) == 0
+        if len(parameters) != len(self.__parameters):
+            raise Exception("Wrong number of parameters for method '%s'. \


Raising bare exceptions is considered a bad practice, see for example https://stackoverflow.com/questions/2052390/manually-raising-throwing-an-exception-in-python#24065533
Here it would be much better to raise a ValueError.

I have modified bare exceptions to more focused ones.

eerovaher · 2021-10-20T16:13:57Z

astroquery/esa/jwst/tests/DummyTapHandler.py

+                    raise Exception("Wrong '%s' parameter value for method '%s'. \
+                    Found: '%s'. Expected: '%s'", (
+                        method_name,
+                        key,
+                        self.__parameters[key],
+                        parameters[key]))


The modern best practice is to use f-strings, so as far as I can tell this could be

Suggested change

raise Exception("Wrong '%s' parameter value for method '%s'. \

Found: '%s'. Expected: '%s'", (

method_name,

key,

self.__parameters[key],

parameters[key]))

raise ValueError(f"Wrong {key} parameter value for method {method_name}. "

f"Found: {self.__parameters[key]}. Expected: {parameters[key]}")

I have replaced most of long strings with f-strings.

eerovaher · 2021-10-20T16:18:18Z

astroquery/esa/jwst/tests/setup_package.py

@@ -0,0 +1,20 @@
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+from __future__ import absolute_import


This shouldn't be needed in Python3.

Suggested change

from __future__ import absolute_import

See also #1864

eerovaher · 2021-10-20T16:22:00Z

astroquery/esa/jwst/tests/test_jwst_remote_data.py

+
+import os
+import tempfile
+import unittest


The tests shouldn't be using unittest, see #861

Removed unittests, but MagicMock still needs to be replaced.

eerovaher · 2021-10-20T16:34:12Z

astroquery/esa/jwst/core.py

+    JWST_MAIN_TABLE = conf.JWST_MAIN_TABLE
+    JWST_OBSERVATION_TABLE = conf.JWST_OBSERVATION_TABLE
+    JWST_OBS_MEMBER_TABLE = conf.JWST_OBS_MEMBER_TABLE
+    JWST_MAIN_TABLE_RA = conf.JWST_MAIN_TABLE_RA
+    JWST_MAIN_TABLE_DEC = conf.JWST_MAIN_TABLE_DEC
+    JWST_PLANE_TABLE = conf.JWST_PLANE_TABLE
+    JWST_ARTIFACT_TABLE = conf.JWST_ARTIFACT_TABLE


Setting up class attributes like this is what the Astroquery API Specification seems to suggest, but the Astropy configuration documentation explicitly warns against this and also provides examples of how config items should be used. See also #544.

Removed following Astropy recommendations.

jespinosaar · 2021-10-21T10:37:03Z

Many thanks for your comments, @eerovaher! I have implemented the requested changes, please let me know if I should modify anything else (e.g. I am working in removing the MagicMock from tests in test_jwsttap.py file, I need to create different mocks providing different results for the same method). I can see we still have the issue with oldest dependencies...
I have a question, JWST launch will be in December, but we would like to have this code merged at the end of November (no data will be accessible, of course). Would it be also possible to have an official astroquery release with this new module?

Thanks!

bsipocz · 2021-10-22T02:00:25Z

I have a question, JWST launch will be in December, but we would like to have this code merged at the end of November (no data will be accessible, of course). Would it be also possible to have an official astroquery release with this new module?

I'm planning to do a tagged release at the end of October to address the JPL API changes. But also happy to tag one once this is in, before the launch.
(We will also have dev version release on pypi sooner (e.g. can have that with the merge commit), but I agree that this module will be a significant addition that deserves a tagged release that ends up on conda-forge, etc, too.)

bsipocz · 2021-10-22T02:07:13Z

Re unrelated CI failures: I can't recall seeing this one, but a rebase should certainly resolve the issue.

bsipocz · 2021-10-22T02:16:36Z

Re tests: none of the remote tests seems to work, is there a way to run them?

Also, please let us know if this is ready for review. I suspect it is, but given it's still a draft PR I haven't yet looked into it thoroughly.

bsipocz

Very rough first review. The main point is that I think a lot of the code could be simplified and duplicates removed, especially with omitting the repetition of async and sync jobs with e.g. the usage of the decorator.

bsipocz · 2021-10-22T02:01:39Z

astroquery/esa/jwst/__init__.py

+    """
+
+    JWST_MAIN_TABLE = _config.ConfigItem("jwst.main",
+                                         "JWST main table, combination of \


no need for linebreak, we don't enforce 80 char long lines, but it can go longer (100 for sure, but up to 120 is acceptable)

astroquery/esa/jwst/core.py

bsipocz · 2021-10-22T02:05:36Z

astroquery/esa/jwst/core.py

+        """
+        return self.__jwsttap.load_table(table, verbose)
+
+    def launch_job(self, query, name=None, output_file=None,


Use kwarg only arguments for the optional args, it will be cleaner and easier later to add new ones, etc.
This comment is valid for all the methods, but I won't repeat it elsewhere in this review round.

Suggested change

def launch_job(self, query, name=None, output_file=None,

def launch_job(self, query, *, name=None, output_file=None,

bsipocz · 2021-10-22T02:11:21Z

astroquery/esa/jwst/core.py

+JWST TAP plus
+=============
+
+@author: Raul Gutierrez-Sanchez


This seems to be already outdated. I would suggest removing it (we're planning to clean up the rest of the modules, too). The git history tracks code credits more accurately, than just the name and date of the first version of a given file.

(And for the longer term we already have a WIP PR to name people in charge for the different modules)

bsipocz · 2021-10-22T02:12:16Z

astroquery/esa/jwst/core.py

+    ARTIFACT_PRODUCT_TYPES = ['info', 'thumbnail', 'auxiliary', 'science',
+                              'preview']
+    INSTRUMENT_NAMES = ['NIRISS', 'NIRSPEC', 'NIRCAM', 'MIRI', 'FGS']
+    TARGET_RESOLVERS = ['ALL', 'SIMBAD', 'NED', 'VIZIER']


I love how this module relies on the other parts of the library 🎉

bsipocz · 2021-10-22T02:18:05Z

astroquery/esa/jwst/core.py

+                     f"{filter_name_cond}"
+                     f"{props_id_cond}"
+                     f"ORDER BY dist ASC")
+            print(query)


remove the prints

bsipocz · 2021-10-22T02:18:54Z

astroquery/esa/jwst/core.py

+        -------
+        The job results (astropy.table).
+        """
+        return self.__query_region(coordinate, radius, width, height,


why is this necessary? why isn't __query_region is `query_region``?

Same comment above, we can reduce the methods and use just one "query_region" with a boolean parameter async with False as default value to trigger the job synchronously or asynchronously (as it is already done in ESA HST module), instead of using async_to_sync class decorator. What do you think @bsipocz ?

bsipocz · 2021-10-22T02:22:37Z

docs/esa/jwst.rst

@@ -0,0 +1,750 @@
+.. doctest-skip-all


we do support doctesting narrative docs with remote-data, please consider using it (though without data it's probably not yet super relevant here)

bsipocz · 2021-10-22T02:23:43Z

astroquery/esa/jwst/tests/test_jwstdata.py

+    return mp
+
+
+class TestData(unittest.TestCase):


it would be nicer to not use unittest, but be pure pytest only, but this won't be a blocker

jespinosaar · 2021-10-25T15:16:37Z

Thanks for the review @bsipocz ! I am implementing these changes and simplifying the code, I will let you know when it is ready.
Given that we want to merge it by the end of November (thanks for the possibility of having a tagged release), I think this is ready to review, so I will change it to PR.
Regarding the remote tests, I am going to remove them. They are failing because I have implemented dummy URLs, we do not want our operational URL to become public until we empty our database prior to launch. But, in any case, as it will be empty, the remote tests will still fail, so we will include them in further updates, what do you think?
Finally, I will rebase the code in the next update, let's see if it is solved...

keflavich · 2021-10-25T15:34:10Z

@jespinosaar rather than delete the tests, mark them as "expected failures" (xfail) with an explanation that they should start passing when the database goes online.

jespinosaar · 2021-10-25T16:07:47Z

@jespinosaar rather than delete the tests, mark them as "expected failures" (xfail) with an explanation that they should start passing when the database goes online.

The point is that, as the observations appearing in those tests will be removed new ones will be ingested, they are going to fail until we modify them, so I see no reason to keep them, at least, until we have a basic set of observations and files to reference the tests. What do you think @keflavich ?

keflavich · 2021-10-25T17:25:02Z

Right, ok, if the tests will never pass as written, it makes sense to remove them.

bsipocz · 2021-10-25T19:11:28Z

The point is that, as the observations appearing in those tests will be removed new ones will be ingested, they are going to fail until we modify them, so I see no reason to keep them, at least, until we have a basic set of observations and files to reference the tests. What do you think @keflavich ?

this case I think it's fine to remove the current remote tests. If you currently have some simulated data in the archive, etc, I think it would still be preferable to have the tests using those, just to make sure the module works as expected. If the archive is totally empty and real data is expected to be very different, then remove is the only way ahead.

jespinosaar · 2021-11-26T18:18:41Z

Thanks a lot for your help on this @bsipocz and @eerovaher ! Well, we are progressing!

I think I can solve this error, as this is now an assertion in one of the messages. I have splitted the test_query_target test in two parts, the first one (test_query_target_errors) with the errors and the second one (test_query_target) with the correct inputs. The first test is working with your fix, but the second one is trying to connect to internet... I have commented this second test, I think I should find a less complex way of testing this method.

@bsipocz. I can upload this fix now, what do you think?

bsipocz · 2021-11-26T18:21:28Z

@jespinosaar - yes, please go ahead, I got side tracked into some cleanup tasks while we were waiting on CI :)

bsipocz · 2021-11-26T18:27:55Z

astroquery/esa/jwst/tests/test_jwsttap.py

-            dummyTapHandler.check_call('launch_job', parameters)
+        assert 'This target name cannot be determined with this resolver: VIZIER' in err.value.args[0]
+
+    # def test_query_target(self):


Please do rather delete it than comment out, unless we're putting this back in once the remote tests are added with the functional archive

Ok, I am going to remove it.

codecov · 2021-11-26T18:31:14Z

Codecov Report

Merging #2140 (c84884e) into main (0833f63) will increase coverage by 0.29%.
The diff coverage is 69.80%.

@@            Coverage Diff             @@
##             main    #2140      +/-   ##
==========================================
+ Coverage   61.90%   62.19%   +0.29%     
==========================================
  Files         129      131       +2     
  Lines       16310    16777     +467     
==========================================
+ Hits        10097    10435     +338     
- Misses       6213     6342     +129

Impacted Files	Coverage Δ
astroquery/esa/jwst/core.py	`69.67% <69.67%> (ø)`
astroquery/esa/jwst/data_access.py	`75.00% <75.00%> (ø)`
astroquery/utils/tap/core.py	`41.61% <0.00%> (+0.19%)`	⬆️
astroquery/simbad/core.py	`90.84% <0.00%> (+1.11%)`	⬆️
astroquery/utils/tap/taputils.py	`79.54% <0.00%> (+5.68%)`	⬆️

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 0833f63...c84884e. Read the comment docs.

jespinosaar · 2021-11-26T18:54:27Z

Ok, now it seems everything is finally working!

bsipocz · 2021-11-26T19:10:56Z

I've double-checked all the modules this uses, and run the remotes, too. test_query_target_error fails if I run the tests remotely, but it's OK as it's related to the dummy URL. So let's go ahead and merge!

bsipocz · 2021-11-26T19:11:17Z

Thank you @jespinosaar and everyone else involved!

jespinosaar · 2021-11-26T19:19:22Z

Many thanks to everyone for your cooperation and support!! See you when we update the URL and release eJWST!

saimn · 2021-11-27T17:07:03Z

ideally, we should keep the module level tests separate, they should run in separate contexts. Not sure whether that's easily possible with pytest. @saimn would know I think.

@bsipocz - Unless you run tests for each modules in a separate process I don't think there is an easy solution.

jespinosaar changed the title ~~Ejwst prelaunch~~ eJWST prelaunch Aug 23, 2021

keflavich added the New Service label Aug 23, 2021

jespinosaar force-pushed the ejwst-prelaunch branch from 94afe28 to da6c017 Compare October 19, 2021 15:56

eerovaher reviewed Oct 20, 2021

View reviewed changes

bsipocz added this to the v0.4.5 milestone Oct 22, 2021

bsipocz requested changes Oct 22, 2021

View reviewed changes

bsipocz mentioned this pull request Oct 22, 2021

Make it easier to change Gaia row limit #2184

Open

jespinosaar marked this pull request as ready for review October 25, 2021 15:19

jespinosaar force-pushed the ejwst-prelaunch branch from 3e3b37d to 1dd6961 Compare November 2, 2021 12:23

jespinosaar and others added 16 commits November 26, 2021 09:57

JWSTPCR-179: e2e tests and remote_data tested

a22239f

JWSTPCR-179: e2e tests and JWSTPCR-184: update to CAOM 2.4

7b2e742

JWSTPCR-135: MAST token in login, login_gui, set_token and documentation

6a905ae

JWSTPCR-135: rephrasing docs

334ce6f

Style fixes

f418bf2

Deprecated np.object in tests

1ec17eb

Remove old folder

d90ae94

Ned import updated

2942d87

Optimized imports

391f61c

JWSTPCR-254: remove copied code from TAP

01c38d5

JWSTPCR-254: code compatibility issues

0849e25

JWSTPCR-254: sync/async methods simplified, comments on PR#2140

dea454c

Changes.rst updated

959bac6

Fixes for PR 2140

63987c7

Adding comments to config items to fix parser issues

c8c19c6

Fix class instance mocking

ca4ddc1

bsipocz force-pushed the ejwst-prelaunch branch from e572ac2 to ca4ddc1 Compare November 26, 2021 17:58

Test_query_target_error with correct assertions

4d4f583

bsipocz reviewed Nov 26, 2021

View reviewed changes

Commented test deleted

c84884e

bsipocz merged commit ce8cc52 into astropy:main Nov 26, 2021

jespinosaar deleted the ejwst-prelaunch branch December 3, 2021 09:58

		from astroquery.utils.tap.model.job import Job


		class DummyTapHandler(object):

		@@ -0,0 +1,20 @@
		# Licensed under a 3-clause BSD style license - see LICENSE.rst
		from __future__ import absolute_import

	def launch_job(self, query, name=None, output_file=None,
	def launch_job(self, query, *, name=None, output_file=None,

eJWST prelaunch #2140

eJWST prelaunch #2140

Conversation

jespinosaar commented Aug 23, 2021

pep8speaks commented Aug 23, 2021 • edited

Comment last updated at 2021-11-26 18:38:01 UTC

keflavich commented Aug 23, 2021

jespinosaar commented Aug 24, 2021

jespinosaar commented Oct 19, 2021

keflavich commented Oct 19, 2021

eerovaher commented Oct 19, 2021

jespinosaar commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jespinosaar commented Oct 21, 2021

bsipocz commented Oct 22, 2021

bsipocz commented Oct 22, 2021

bsipocz commented Oct 22, 2021 • edited

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jespinosaar commented Oct 25, 2021

keflavich commented Oct 25, 2021

jespinosaar commented Oct 25, 2021

keflavich commented Oct 25, 2021

bsipocz commented Oct 25, 2021

jespinosaar commented Nov 26, 2021 • edited

bsipocz commented Nov 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 26, 2021 • edited

Codecov Report

jespinosaar commented Nov 26, 2021

bsipocz commented Nov 26, 2021

bsipocz commented Nov 26, 2021

jespinosaar commented Nov 26, 2021

saimn commented Nov 27, 2021

pep8speaks commented Aug 23, 2021 •

edited

bsipocz commented Oct 22, 2021 •

edited

jespinosaar commented Nov 26, 2021 •

edited

codecov bot commented Nov 26, 2021 •

edited