Compatibility with parfive 1.1

[Raahul-Singh]

Created to verify the new parfive release against sunpy's CI.

(Also fixes #3844 as a side effect)

[Raahul-Singh]

I find it curious that python3.8 tests should fail. I read somewhere that they are changing the way mock is used in 3.8. Could that be the reason?

[Cadair]

It's not python 3.8, the only online test build happens to be the python 3.8 one.

[Cadair]

There looks to be a real issue with the example gallery and one of the online test issues look real (the SUVI tests are a known issue).

[Raahul-Singh]

You were right @Cadair setting the max_splits = 1 solved the JSOC errors. NORH error remain now.
In parfive, max_splits is set to be 12 by default.

[Cadair]

I am chatting to the people who maintain JSOC, we might want to reduce the default from 12 in parfive (12*5 in parfive leads to a default of 60 connections, which seems like a lot!). However, should we also consider if we can override the default reliably in the JSOC client?

[Cadair]

Conclusion with the JSOC folk is we probably shouldn't be hammering them with a lot of open connections, I propose that the JSOCClient defaults max_splits to two, and keeps max_conn at 5.

[Raahul-Singh]

I am chatting to the people who maintain JSOC, we might want to reduce the default from 12 in parfive (12*5 in parfive leads to a default of 60 connections, which seems like a lot!). However, should we also consider if we can override the default reliably in the JSOC client?

That is what I did here. Set the default to 1 and the tests worked. For this, we can simply implement max_conns in JSOCClient.fetch and pass it to JSOCClient.get_request as a kwarg.

[Raahul-Singh]

Conclusion with the JSOC folk is we probably shouldn't be hammering them with a lot of open connections, I propose that the JSOCClient defaults max_splits to two, and keeps max_conn at 5.

Doesn't max_splits override max_conn ?

[Cadair]

Doesn't max_splits override max_conn ?

In a series of poor naming choices, max_conn is the number of parallel files that will be downloaded and max_splits is the number of parts used for each file.

We should override the max_splits default in JSOCClient but allow users to override it I would think.

[Raahul-Singh]

One of the two NORH test now works. The other should have passed as well. Oh, and NOAA test felt lonely and decided it will fail all of a sudden for no reason whatsoever.

[Cadair]

Latest intel on JSOC is that the max connection limit is 10 per IP. So I suggest we go for max_splits=2 max_conn=4 so we run at 8 not the max of 10.

[Raahul-Singh]

I've applied your suggestions for JSOC. NORH fails despite the changes

[Cadair]

I wasn't really suggesting it would be a fix for NORH.

I think we should allow the user, through the JSOCClient.fetch() method, to override these keyword arguments and all the other arguments to enqueue_file via **kwargs.

A pattern like:

def func(**kwargs):
    defaults = {'max_conn': 4}
    defaults.update(kwargs)
    enqueue_file(**defaults)

(all be it with better variable names) works well for this situation.

[pep8speaks]

Hello @Raahul-Singh! 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 2020-04-15 17:19:48 UTC

[Raahul-Singh]

I wasn't really suggesting it would be a fix for NORH.

Yeah I know. Was just wondering if I'll get lucky and it'll fix this too. 😁

I think we should allow the user, through the JSOCClient.fetch() method, to override these keyword arguments and all the other arguments to enqueue_file via **kwargs.

A pattern like:

def func(**kwargs):
    defaults = {'max_conn': 4}
    defaults.update(kwargs)
    enqueue_file(**defaults)

(all be it with better variable names) works well for this situation.

I have applied the changes. However, the change of the variable names would need to be done first in parfive.

[Cadair]

the change of the variable names

oh I was mostly referring to defaults lol

[Raahul-Singh]

The NORH test fails with the following message :
Waiting for ('230', '33x') but got 530 [" Can't change from guest user."]
But even with parfive==1.0.0, it doesn't download the files and returns an empty list (locally).

[Raahul-Singh]

Any idea why the database tests would be failing?

[Raahul-Singh]

The test_get_request test fails because I am adding max_splits through fetch and not through get_request.
Should I change the test or add a default max_splits to get_request as well?
Perhaps check the kwargs before using max_splits ?

[nabobalis]

The doc build has downloaded the example data so far, have to see if the example fails again.

[nabobalis]

The doc build passed. It must have been trainsist.

[Raahul-Singh]

@Cadair These failed jsoc tests are because I switched the parfive version from 1.1rc2 to >=1.0 ?

[Cadair]

Well we definitely shouldn't have broken support for 1.0 😟

[Raahul-Singh]

Well we definitely shouldn't have broken support for 1.0

Most likely because of the new kwargs that we've added to JSOC and are passing on parfive, I believe.

Comments addressed.

[nabobalis]

Devdeps failed on downloading a file in an example.

[nabobalis]

Thanks again @Raahul-Singh

[sunpy-backport]

The backport to 1.0 failed:

Commits ["69284c2fe5d06231a109bb2813b8e9f7b73e692b","ed15986cc925fc112843769c457cb23f9c45f346","1e79edd0be5fa1bd01ff1443c49dc722d711ae35","17f2e5c77b60517a76cd7e6c2ee2e015a590aaea"] could not be cherry-picked on top of 1.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 1.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 69284c2fe5d06231a109bb2813b8e9f7b73e692b ed15986cc925fc112843769c457cb23f9c45f346 1e79edd0be5fa1bd01ff1443c49dc722d711ae35 17f2e5c77b60517a76cd7e6c2ee2e015a590aaea
# Create a new branch with these backported commits.
git checkout -b backport-3822-to-1.0
# Push it to GitHub.
git push --set-upstream origin backport-3822-to-1.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 1.0 and the compare/head branch is backport-3822-to-1.0.

[sunpy-backport]

The backport to 1.1 failed:

Commits ["69284c2fe5d06231a109bb2813b8e9f7b73e692b","ed15986cc925fc112843769c457cb23f9c45f346","1e79edd0be5fa1bd01ff1443c49dc722d711ae35","17f2e5c77b60517a76cd7e6c2ee2e015a590aaea"] could not be cherry-picked on top of 1.1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 1.1
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 69284c2fe5d06231a109bb2813b8e9f7b73e692b ed15986cc925fc112843769c457cb23f9c45f346 1e79edd0be5fa1bd01ff1443c49dc722d711ae35 17f2e5c77b60517a76cd7e6c2ee2e015a590aaea
# Create a new branch with these backported commits.
git checkout -b backport-3822-to-1.1
# Push it to GitHub.
git push --set-upstream origin backport-3822-to-1.1
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 1.1 and the compare/head branch is backport-3822-to-1.1.