Gaia astroquery 1.1 #2376

mhsarmiento · 2022-04-25T16:57:09Z

Dear Astroquery Team,
This pull request contains a new feature in the Gaia module and an update in the default value of one of the parameters of the method 'load_data'

Now the users of the Astroquery/Gaia module will know if there is a planned downtime of the Archive or if there is any issue on-going on the Gaia archive.
For the method 'load_data' it has been updated the default value of the parameter 'valid_data' to 'False'. With this change the epoch photometry service will returns all data associated to a given source. This change also mimics the behaviours of the current UI of the archive.

Thank you in advance,
all the best,
Maria

… the gaia module [astropy#2376]

codecov · 2022-04-25T18:04:48Z

Codecov Report

Merging #2376 (fc8d45b) into main (9526b6f) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #2376   +/-   ##
=======================================
  Coverage   62.92%   62.92%           
=======================================
  Files         133      133           
  Lines       17269    17289   +20     
=======================================
+ Hits        10866    10879   +13     
- Misses       6403     6410    +7

Impacted Files	Coverage Δ
astroquery/gaia/core.py	`71.47% <66.66%> (-0.47%)`	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

bsipocz

There are some commit duplications and merge commits, please rebase and maybe squash them down a bit, too.

bsipocz · 2022-04-25T21:16:22Z

astroquery/gaia/core.py

@@ -911,5 +915,20 @@ def launch_job_async(self, query, name=None, output_file=None,
                                        upload_table_name=upload_table_name,
                                        autorun=autorun)

+    def get_status_messages(self):
+        """Retrieve the messages to inform users about
+        the status of JWST TAP


Suggested change

the status of JWST TAP

the status of Gaia TAP

Updated. Thank you

bsipocz · 2022-04-25T21:23:55Z

astroquery/gaia/core.py

@@ -74,6 +74,10 @@ def __init__(self, tap_plus_conn_handler=None,
        else:
            self.__gaiadata = datalink_handler

+        # Enable notifications
+        if show_messages:


Please add a test with show_messages=True case that parses a dummy response with a message.

Test added with show_message=True

bsipocz · 2022-04-25T21:26:42Z

astroquery/gaia/core.py

@@ -51,7 +51,7 @@ def __init__(self, tap_plus_conn_handler=None,
                 gaia_data_server='https://gea.esac.esa.int/',
                 tap_server_context="tap-server",
                 data_server_context="data-server",
-                 verbose=False):
+                 verbose=False, show_messages=True):


Somewhat tangential to this PR, but it would be great if all these __init__ parameters had a short docstring. And you could also consider making them keyword only arguments.

Thanks for your comment. I have brought your question to the team for internal discussion. We will came back to you soon.

I recommend you replace show_messages with something more descriptive so that it would be more obvious how it differs from verbose.

bsipocz · 2022-04-25T21:29:35Z

CHANGES.rst

-casda
-^^^^^
-
- Add the ability to produce 2D and 3D cutouts from ASKAP images and cubes. [#2366]


This should not be removed

Restored! Thanks

The statement about not removing change log entries applies to all of them, not just this one entry individually.

@eerovaher - This is a rebase issue, I can fix it when the PR otherwise ready to be merged rather than nitpick on the need of multiple round of rebases.

bsipocz · 2022-04-26T00:28:20Z

astroquery/gaia/__init__.py

@@ -39,6 +39,8 @@ class Conf(_config.ConfigNamespace):
                                      'MCMC_GSPPHOT',
                                      'MCMC_MSC']

+    GAIA_MESSAGES = _config.ConfigItem("notification?action=GetNotifications", "Gaia Messages")


What are the other possibilities for this configitem?

Sorry. Not sure if I have understood your question. This is the way we use to add a modifier to the request.

If this configuration item can only take one value then it is not configurable and should not be managed by the configuration system. So if you have implemented this as a configuration item then there must be more values it can take, but they are not documented anywhere.

This has to be deleted now

bsipocz · 2022-04-26T00:30:05Z

astroquery/gaia/core.py

+                    string_message = line.decode("utf-8")
+                    print(string_message[string_message.index('=') + 1:])
+        except OSError as e:
+            print("Status messages could not be retrieved")


Why the try/except, why not raise this as an exception instead? Is it expected to be unstable?

Also, no need for catching the exception (as e) if you don't use it later.

updated. Thank you!

eerovaher · 2022-04-26T12:53:25Z

astroquery/gaia/core.py

+                    string_message = line.decode("utf-8")
+                    print(string_message[string_message.index('=') + 1:])


Equivalent, but shorter and clearer code:

Suggested change

string_message = line.decode("utf-8")

print(string_message[string_message.index('=') + 1:])

print(line.decode("utf-8").split('=', 1)[1])

thanks for your inputs. Updated!

… the gaia module [astropy#2376]

bsipocz · 2022-05-24T19:18:05Z

A rebase went wrong here. Make sure you rebase on astropy/main instead of the main of your fork or esdc's fork.

pep8speaks · 2022-05-24T21:20:08Z

Hello @mhsarmiento! 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 2022-06-13 15:46:49 UTC

bsipocz · 2022-05-24T21:31:20Z

A rebase went wrong here. Make sure you rebase on astropy/main instead of the main of your fork or esdc's fork.

OK, so I correct this to:

A rebase went wrong here. Make sure you rebase on the freshly fetched astropy/main instead of the main of your fork or esdc's fork.

eerovaher · 2022-05-25T13:08:09Z

astroquery/gaia/core.py

+                        except ValueError as e:
+                            print(e)
+                            pass
+        except OSError:


It is best to use the most specific error type possible. In this case it looks like ConnectionError or perhaps even one of its subclasses would be more appropriate.

I have opened a internal ticket to review your proposal

eerovaher · 2022-05-25T13:11:38Z

astroquery/gaia/tests/test_gaiatap.py

@@ -14,6 +14,7 @@


 """
+import unittest


The use of unittest was just removed by 6ec71ce, it should not be reintroduced without a very good reason.

eerovaher · 2022-05-25T13:16:34Z

This pull request makes unannounced and unrelated changes to the XMM-Newton sub-package. It would be better to open a separate pull request for those.

bsipocz · 2022-05-25T16:54:26Z

This pull request makes unannounced and unrelated changes to the XMM-Newton sub-package. It would be better to open a separate pull request for those.

Ideally these should indeed be separate PRs, but at this point I don't think we should separate them and manage two parallel PRs. The changelog will need a fixing anyway, but that can be done as a last step.

As for the review, I'll try to get back to it this week or the next.

jespinosaar · 2022-05-31T15:16:11Z

I have rebased again and removed the unnecessary commits, @mhsarmiento you can now proceed with the required changes.

cc @esdc-esac-esa-int

mhsarmiento · 2022-05-31T18:07:14Z

I have rebased again and removed the unnecessary commits, @mhsarmiento you can now proceed with the required changes.

cc @esdc-esac-esa-int

done! thanks a lot for your reviewing the rebase and the xmm changes @jespinosaar

mhsarmiento · 2022-05-31T18:17:14Z

Thanks @bsipocz and @eerovaher for your support to the Gaia module in Astroquery. Your comments are very helpful and will be also applied to other ESA projects in Astrouquery. Those projects already pushed the same code to Astroquery to show the TAP notifications to the users (for example at esa/jwst module).

bsipocz · 2022-05-31T19:10:34Z

I did some investigating to see what went wrong with the previous rebases and concluded that it was rebased on esdc-esac-esa-int/main instead of astropy/main.
The issue is that the esdc-esac-esa-int/main is not tracking the canonical copy, but frozen in time to the point when the fork was created. So whenever you rebased on that branch, it will pull in a lot of extra commits and causing a conflicting nightmare.

My assumption is because of the confusing nature of calling the personal fork and these origin/upstream. I think that this git default naming is unfortunate and not is not fit for purpose for any workflow that deals with multiple accounts and is therefore a cause of great pain.

So to fix the workflow my suggestion is to rename all the remotes you work with to reflect the user/organization name on github. That way it's always clear which versions you are using as e.g. the base of a rebase. The workflow for a rebase, in that case, would be:

# check that all the remotes are set
$ git remote -v

astropy	git@github.com:astropy/astroquery.git (fetch)
astropy	git@github.com:astropy/astroquery.git (push)
esdc-esac-esa-int	git@github.com:esdc-esac-esa-int/astroquery (fetch)
esdc-esac-esa-int	git@github.com:esdc-esac-esa-int/astroquery (push)

# check that you're on the correct feature branch
$ git branch --show-current

gaia-astroquery-1.1

# Update all remotes. If you list all the branches, this command will update what's in `remotes/<username>/<branchname>`

$ git fetch --all  

Fetching astropy
Fetching esdc-esac-esa-int

$ # Optionally remove all outdated branches as well with the following. (also, strictly speaking --all is not mandatory, but I think it's good practice)
$ # git fetch --all --prune

# Do the rebase, it pops up the editor, select commits for squash/removal/move around. Fix conflicts and keep doing `git rebase --continue` until done.
$ git -rebase -i astropy/main

# when done, force push it to esdc... You don't need to be, but I think it's a good practice to be specific.
$ git push esdc-esac-esa-int HEAD:<name_of_branch> --force

Hope it helps.

(of course, the other approach is what you did, is to stash all commits done to one, but that also removes all granularity in the PR that may be useful later when one tries to trace the origin of a bug (which can be a bit problematic for very large PRs))

… the gaia module [astropy#2376]

bsipocz · 2022-06-01T06:28:56Z

We already have the remotes with different names and a well-defined procedure documented, so maybe this has been just a small slip.

no worries, this takes time and practice for everyone, I feel the workflow got much easier already (at least on the reviewer end), so thank you!

bsipocz · 2022-06-01T06:49:45Z

astroquery/gaia/core.py

+        the status of Gaia TAP
+        """
+        try:
+            print("parsing notification messages")


I know that many users don't like default verbosity. OTOH from the archive's side, I understand why these messages are desirable.
So maybe a middle ground here to add another sentence to say how to turn it off?

Also, add a short paragraph about it in the narrative docs, users should know how to turn this off.

This call to our TAP will tell the user if the TAP is under maintenance or it is not working, so they know if what they will do will be working in advance. But it is ok, we can add in the documentation how to turn it off. @mhsarmiento, could you please add this in the documentation of the python file and the .rst file?

I know that many users don't like default verbosity. OTOH from the archive's side, I understand why these messages are desirable. So maybe a middle ground here to add another sentence to say how to turn it off?

Also, add a short paragraph about it in the narrative docs, users should know how to turn this off.

Hi @bsipocz , yes, the same code to show messages from the tap sever (before implementing the changes proposed in this PR) , can be found at esa/jwst/core.py line 681. In fact, the code implemented in Gaia was based on that. This is the reason because I was saying that your comments are really useful. We can use your comments to correct also this method under esa/jwst/core.py but in another PR

bsipocz · 2022-06-01T06:50:08Z

The documentation needs to be updated, because the current code examples do not expect any notifications from the server, and I would rename the new show_messages argument to something like show_server_notifications so that it would be clear how it differs from the already existing verbose argument.

@mhsarmiento , why not 'show_tap_notifications'?

I don't think we have a precedent yet, in other modules (maybe @keflavich you can recall something). Do we know of any other servers where this would be useful? But I would suggest using something that can be reused generally for the other, non-tap modules, too. I don't really have a strong preference, beyond the potential reusability of the name for other modules. What about show_server_messages?

jespinosaar · 2022-06-01T06:58:40Z

It is ok from our side.

… the gaia module [astropy#2376]

eerovaher · 2022-06-07T20:19:27Z

astroquery/gaia/core.py

+                        except ValueError as e:
+                            print(e)
+                            pass
+                        except IndexError as e:
+                            print("Archive down for maintenance")
+                            pass


pass is a null operation which is meant for situations where the Python syntax requires an indented code block, but no code should be executed. There is no need to use pass here because the print() calls form the indented code blocks that Python expects. Furthermore, in the case of an IndexError, e would be an unused variable.

Suggested change

except ValueError as e:

print(e)

pass

except IndexError as e:

print("Archive down for maintenance")

pass

except ValueError as e:

print(e)

except IndexError:

print("Archive down for maintenance")

Implemented. Thanks for the tip

…/gaia/core.py, method 'def get_status_messages'

mhsarmiento · 2022-06-09T14:25:29Z

The only remaining item is the GAIA_MESSAGES conf item. Is there any situation where a user is expected to interact with it? If not, then hardwire it in core.py rather than as a confitem.

Thanks @bsipocz!. GAIA_MESSAGES has been removed as conf item and hardwired at 'core.py'. If in the future there is a situation in which the user can interact with that we will put it back as conf item but for now there isn't any plan for that.
Thanks again for your help.

bsipocz

One more comment, otherwise I think this is good to go. (The archive is now down, so I see test failures with the docs, but that is unrelated to this PR, so address any actual issues that may come up separately when DR3 is out.)

bsipocz · 2022-06-13T00:18:57Z

astroquery/gaia/__init__.py

@@ -39,6 +39,8 @@ class Conf(_config.ConfigNamespace):
                                      'MCMC_GSPPHOT',
                                      'MCMC_MSC']

+    GAIA_MESSAGES = _config.ConfigItem("notification?action=GetNotifications", "Gaia Messages")


This has to be deleted now

bsipocz · 2022-06-13T00:26:42Z

I'm cleaning up the duplicate merges etc, and will do the init cleanups and merge this.

…ations service to notify users of service availability problems

…etrieval_Type = "ALL" VS. Retrieval_type = "Epoch Photometry"

…oad_data'

… the gaia module [astropy#2376]

…/gaia/core.py, method 'def get_status_messages'

Renamed "show_messages" by "show_server_messages" Corrected test case as it was not contemplating the user case in which Gaia archive is down for maintenance.

mhsarmiento · 2022-06-13T07:47:54Z

One more comment, otherwise I think this is good to go. (The archive is now down, so I see test failures with the docs, but that is unrelated to this PR, so address any actual issues that may come up separately when DR3 is out.)

Thanks! I think that I forgot to commit this file. sorry for that.

Yes, today at 12h CEST DR3 will be out! Very exciting times! :-)

bsipocz · 2022-06-13T16:17:50Z

Thank you @mhsarmiento!

jespinosaar · 2022-06-14T12:00:09Z

Many thanks @bsipocz and @mhsarmiento !

bsipocz · 2022-06-14T19:28:19Z

@mhsarmiento @jespinosaar - should we change the default from DR2 to DR3 or it's too early?
(I expect a swarm of issues coming in about it, at least that happened between DR1 and DR2)

mhsarmiento · 2022-06-14T20:08:56Z

@mhsarmiento @jespinosaar - should we change the default from DR2 to DR3 or it's too early? (I expect a swarm of issues coming in about it, at least that happened between DR1 and DR2)

Hi! Yes, it should be changed from DR2 to DR3 as we have done the same the UI of GACS. Thanks for spotting this out

mhsarmientoc pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2022

This new commit updates the file CHANGES.rst with the changes made in…

2c082bf

… the gaia module [astropy#2376]

mhsarmientoc pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2022

This new commit updates the file CHANGES.rst with the changes made in…

6df03ad

… the gaia module [astropy#2376]

bsipocz added the gaia label Apr 25, 2022

bsipocz requested changes Apr 26, 2022

View reviewed changes

eerovaher reviewed Apr 26, 2022

View reviewed changes

mhsarmiento added a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 2022

Implement comments to PR astropy#2376

3eeb177

mhsarmiento force-pushed the gaia-astroquery-1.1 branch from 3ff27d2 to b66f849 Compare May 24, 2022 18:46

mhsarmiento added a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 2022

This new commit updates the file CHANGES.rst with the changes made in…

45a06c0

… the gaia module [astropy#2376]

mhsarmiento added a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 2022

This new commit updates the file CHANGES.rst with the changes made in…

1a97261

… the gaia module [astropy#2376]

mhsarmiento added a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 2022

Implement comments to PR astropy#2376

08f6f25

mhsarmiento force-pushed the gaia-astroquery-1.1 branch from 239bf98 to 1a321e1 Compare May 24, 2022 21:20

mhsarmiento force-pushed the gaia-astroquery-1.1 branch from 1a321e1 to 64d17fc Compare May 24, 2022 21:35

eerovaher reviewed May 25, 2022

View reviewed changes

jespinosaar force-pushed the gaia-astroquery-1.1 branch from 2ae9491 to b491bf4 Compare May 31, 2022 09:54

jespinosaar added this to the v0.4.7 milestone May 31, 2022

jespinosaar force-pushed the gaia-astroquery-1.1 branch from b491bf4 to e10c47f Compare May 31, 2022 14:49

mhsarmientoc pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 31, 2022

Implemented comments from @eerovaher on PR astropy#2376

8255988

bsipocz pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 31, 2022

This new commit updates the file CHANGES.rst with the changes made in…

cf4ca68

… the gaia module [astropy#2376]

bsipocz pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 31, 2022

Implement comments to PR astropy#2376

681aebb

bsipocz reviewed Jun 1, 2022

View reviewed changes

bsipocz pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Jun 2, 2022

This new commit updates the file CHANGES.rst with the changes made in…

ad9be3e

… the gaia module [astropy#2376]

bsipocz pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Jun 2, 2022

Implement comments to PR astropy#2376

e37fd67

bsipocz force-pushed the gaia-astroquery-1.1 branch from 681aebb to e37fd67 Compare June 2, 2022 01:05

eerovaher reviewed Jun 7, 2022

View reviewed changes

mhsarmientoc pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Jun 7, 2022

As suggested in PR astropy#2376, removed "pass" from lines astroquery…

083cc2f

…/gaia/core.py, method 'def get_status_messages'

bsipocz approved these changes Jun 13, 2022

View reviewed changes

mhsarmiento added 8 commits June 12, 2022 17:30

This new check-in implements GAIAPCR-1094: Astroquery Use TAP notific…

f45190a

…ations service to notify users of service availability problems

This new commit implements C9APP-164 [MUST]: Astroquery + Datalink: R…

46b11d2

…etrieval_Type = "ALL" VS. Retrieval_type = "Epoch Photometry"

GAIAPCR-1151 updated help content for param 'valid_data' at method 'l…

f7dd7d2

…oad_data'

This new commit updates the file CHANGES.rst with the changes made in…

79eceb9

… the gaia module [astropy#2376]

Corrected error lines (252 - 255) from Gaia/core.py

62fc989

Implement comments to PR astropy#2376

5d25430

As suggested in PR astropy#2376, removed "pass" from lines astroquery…

9397b78

…/gaia/core.py, method 'def get_status_messages'

Moved GAIA_MESSAGES from __init__.py to core.py.

3990021

Renamed "show_messages" by "show_server_messages" Corrected test case as it was not contemplating the user case in which Gaia archive is down for maintenance.

bsipocz force-pushed the gaia-astroquery-1.1 branch from d90083f to 3990021 Compare June 13, 2022 00:41

Fix changes indent to pass the sphinx build

fc8d45b

bsipocz merged commit cbe6d20 into astropy:main Jun 13, 2022

mhsarmiento deleted the gaia-astroquery-1.1 branch June 14, 2022 13:29

bsipocz mentioned this pull request Nov 9, 2022

GAIA: Return a clear error message if the server is down #1913

Closed

		string_message = line.decode("utf-8")
		print(string_message[string_message.index('=') + 1:])

	string_message = line.decode("utf-8")
	print(string_message[string_message.index('=') + 1:])
	print(line.decode("utf-8").split('=', 1)[1])

Gaia astroquery 1.1 #2376

Gaia astroquery 1.1 #2376

Conversation

mhsarmiento commented Apr 25, 2022

codecov bot commented Apr 25, 2022 • edited

Codecov Report

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

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

bsipocz commented May 24, 2022

pep8speaks commented May 24, 2022 • edited

Comment last updated at 2022-06-13 15:46:49 UTC

bsipocz commented May 24, 2022

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

eerovaher commented May 25, 2022

bsipocz commented May 25, 2022

jespinosaar commented May 31, 2022

mhsarmiento commented May 31, 2022

mhsarmiento commented May 31, 2022

bsipocz commented May 31, 2022

bsipocz commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Jun 1, 2022

jespinosaar commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsarmiento commented Jun 9, 2022

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Jun 13, 2022

mhsarmiento commented Jun 13, 2022

bsipocz commented Jun 13, 2022

jespinosaar commented Jun 14, 2022

bsipocz commented Jun 14, 2022 • edited

mhsarmiento commented Jun 14, 2022

codecov bot commented Apr 25, 2022 •

edited

pep8speaks commented May 24, 2022 •

edited

bsipocz commented Jun 14, 2022 •

edited