Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Fido.search robust against errors from a single client/s #7625

Open
samaloney opened this issue May 14, 2024 · 2 comments · May be fixed by #7626
Open

Make Fido.search robust against errors from a single client/s #7625

samaloney opened this issue May 14, 2024 · 2 comments · May be fixed by #7626
Labels
Effort Medium Requires a moderate time investment Feature Request New feature wanted! net Affects the net submodule Package Intermediate Requires some knowledge of the internal structure of SunPy Priority Medium Non-urgent action required

Comments

@samaloney
Copy link
Contributor

samaloney commented May 14, 2024

Describe the feature

  • Currently if one client has and error the entire Fido.search crashes which is very annoying especially when it's not the intended client that has the problem.

For example at the time of making this issue the VSO seems to be down but this causes searches for NOAA SRS data to crash while a direct search of the SRS client works.

from sunpy.net import Fido, attrs as a

q = Fido.search(a.Time('2022-12-01', '2022-12-31'), a.Instrument.soon)
---------------------------------------------------------------------------
ConnectionRefusedError                    Traceback (most recent call last)
from sunpy.net.dataretriever.sources.noaa import SRSClient
srs = SRSClient()
srs.search(a.Time('2022-12-01', '2022-12-31'))
srs
<sunpy.net.dataretriever.client.QueryResponse object at 0x159be8310>
       Start Time               End Time        Instrument Physobs Source Provider
----------------------- ----------------------- ---------- ------- ------ --------
2022-12-01 00:00:00.000 2022-12-01 23:59:59.999       SOON     SRS   SWPC     NOAA
2022-12-02 00:00:00.000 2022-12-02 23:59:59.999       SOON     SRS   SWPC     NOAA
2022-12-03 00:00:00.000 2022-12-03 23:59:59.999       SOON     SRS   SWPC     NOAA
2022-12-04 00:00:00.000 2022-12-04 23:59:59.999 

Proposed solution

If a query can be handled by more then one client and at least one client doesn't error return a result object. Vaguely related to #7474

@nabobalis nabobalis added Feature Request New feature wanted! net Affects the net submodule Priority Medium Non-urgent action required Effort Medium Requires a moderate time investment Package Intermediate Requires some knowledge of the internal structure of SunPy labels May 14, 2024
@samaloney samaloney linked a pull request May 14, 2024 that will close this issue
2 tasks
@samaloney
Copy link
Contributor Author

samaloney commented May 16, 2024

Based on some discussion in #7626 current plan would be an add a new .errors property to the UnifiedResponse. Before going to far down the implementation I wanted to sketch out the API.

Given the scenario where two clients, A, B can handle the query but an error occurs in A with query = Fido.search(<....>) and assume query[0] === query['a']

  • query['b'] or query[1] Should work and return client Bs results

  • query['a'] or query[0] Should this raise an error or return the error from the client now contained in the UnifiedResponse.errors

  • query.errors[1] or query.errors['b'] error or return None?

  • query.errors[0] or query.errors['a'] return the error from the client?

  • None of the above?

@Cadair
Copy link
Member

Cadair commented May 16, 2024

I think the UnifiedResponse object should still hold an empty table of results from the client that erred, a query was made no results came back. Anything else would be a massive breaking change and generally "weird" imo.

I like the idea of having the indexes for .errors match the main UnifiedResponse object, so I think None as a placeholder for "no error" is good. The only thing I think should be true is if there's no errors at all bool(query.errors) should give you False so you can if if response.errors:.

So basically I am suggesting no change to the behaviour of things which already exist, but the addition of a .errors. You could modify the __repr__ of UnifiedResponse to show the error, like it does the number or results retrieved and the info url etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort Medium Requires a moderate time investment Feature Request New feature wanted! net Affects the net submodule Package Intermediate Requires some knowledge of the internal structure of SunPy Priority Medium Non-urgent action required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants