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

max conns to JSOC is always 1 #5714

Merged
merged 6 commits into from Dec 9, 2021
Merged

max conns to JSOC is always 1 #5714

merged 6 commits into from Dec 9, 2021

Conversation

nabobalis
Copy link
Contributor

No description provided.

@nabobalis nabobalis added [BugFix] net Affects the net submodule labels Nov 19, 2021
@nabobalis nabobalis marked this pull request as ready for review November 19, 2021 20:35
@nabobalis nabobalis requested a review from a team as a code owner November 19, 2021 20:35
@wtbarnes
Copy link
Member

It seems a bit extreme to force only 1 max_split and max_conn. Is it possible to just make the defaults 1?

@nabobalis
Copy link
Contributor Author

nabobalis commented Nov 19, 2021

JSOC say it should only be one and I think we should not let users change this.

@nabobalis nabobalis requested a review from a team as a code owner November 19, 2021 22:32
@nabobalis nabobalis added the Needs Review Needs reviews before merge label Nov 19, 2021
changelog/5714.bugfix.rst Outdated Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Outdated Show resolved Hide resolved
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to consider the knock-on impacts this has on mixed-client downloads and the general poor experience for people who are now used to parallel downloads even when just downloading from JSOC.

sunpy/net/jsoc/jsoc.py Outdated Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Outdated Show resolved Hide resolved
@Cadair
Copy link
Member

Cadair commented Nov 23, 2021

I am treating this as a bugfix for backporting, if we merge this then a second PR actually removing all these kwargs needs opening.

As this is (with out the max_conn setting through Fido) I am 👍 on this, I have no issue with completely disabling chunked downloads for JSOC at their behest.

That being said, I do wonder if there is any point in the max_conn changes in this PR, as it only affects people directly using the JSOCClient, and it's easy to override even for them by passing through a new Downloader object. Why don't we just remove that option in a not-backport PR by pulling the keyword arg completely?

@Cadair
Copy link
Member

Cadair commented Nov 23, 2021

To be clear, the proper long term fix here is Cadair/parfive#39 where the JSOCClient can say to parfive "only one connection at once to jsoc.stanford.edu please" and it can have 10,000 open connections to somewhere else at the same time for all JSOCClient cares.

@wtbarnes
Copy link
Member

wtbarnes commented Nov 23, 2021

To be clear, the proper long term fix here is Cadair/parfive#39 where the JSOCClient can say to parfive "only one connection at once to jsoc.stanford.edu please" and it can have 10,000 open connections to somewhere else at the same time for all JSOCClient cares.

Is there any reason to not wait on that fix then and just pin to the latest version of parfive or higher?

EDIT: I thought that was a PR, but it is just an issue 😅

@nabobalis
Copy link
Contributor Author

nabobalis commented Nov 23, 2021

That being said, I do wonder if there is any point in the max_conn changes in this PR, as it only affects people directly using the JSOCClient, and it's easy to override even for them by passing through a new Downloader object. Why don't we just remove that option in a not-backport PR by pulling the keyword arg completely?

You mean remove the keyword in the fetch for jsoc or in the downloader?

changelog/5714.bugfix.rst Outdated Show resolved Hide resolved
sunpy/net/fido_factory.py Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Show resolved Hide resolved
sunpy/net/fido_factory.py Outdated Show resolved Hide resolved
sunpy/net/fido_factory.py Outdated Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Show resolved Hide resolved
sunpy/net/fido_factory.py Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Show resolved Hide resolved
sunpy/net/jsoc/jsoc.py Outdated Show resolved Hide resolved
@nabobalis
Copy link
Contributor Author

I will manually backport to each.

@nabobalis nabobalis merged commit 4b26232 into sunpy:main Dec 9, 2021
@nabobalis nabobalis deleted the jsoc branch December 9, 2021 19:42
This was referenced Dec 9, 2021
@dstansby dstansby removed the Needs Review Needs reviews before merge label Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants