Skip to content

Backport PR 10437 to LTS - #10683

Merged
astrofrog merged 2 commits into
astropy:v4.0.xfrom
pllim:backport-pr10437
Oct 9, 2020
Merged

Backport PR 10437 to LTS#10683
astrofrog merged 2 commits into
astropy:v4.0.xfrom
pllim:backport-pr10437

Conversation

@pllim

@pllim pllim commented Aug 27, 2020

Copy link
Copy Markdown
Member

Lock-free cache implementation by @aarchiba in #10437 for LTS.

@pllim pllim added the utils label Aug 27, 2020
@pllim pllim added this to the v4.0.2 milestone Aug 27, 2020
@pep8speaks

pep8speaks commented Aug 27, 2020 •
edited
Loading

Copy link
Copy Markdown

Hello @pllim! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 27:80: E501 line too long (80 > 79 characters)
Line 993:80: E501 line too long (88 > 79 characters)
Line 994:80: E501 line too long (82 > 79 characters)
Line 1188:80: E501 line too long (84 > 79 characters)
Line 1245:80: E501 line too long (83 > 79 characters)
Line 1298:80: E501 line too long (80 > 79 characters)
Line 1520:80: E501 line too long (85 > 79 characters)
Line 1546:80: E501 line too long (94 > 79 characters)
Line 1647:80: E501 line too long (88 > 79 characters)
Line 1669:80: E501 line too long (99 > 79 characters)
Line 1675:80: E501 line too long (93 > 79 characters)
Line 1774:80: E501 line too long (81 > 79 characters)
Line 1784:80: E501 line too long (88 > 79 characters)
Line 1853:80: E501 line too long (97 > 79 characters)
Line 1854:80: E501 line too long (85 > 79 characters)

Line 1055:80: E501 line too long (85 > 79 characters)
Line 1062:80: E501 line too long (81 > 79 characters)
Line 1970:80: E501 line too long (92 > 79 characters)
Line 1980:80: E501 line too long (92 > 79 characters)
Line 2033:80: E501 line too long (87 > 79 characters)
Line 2084:80: E501 line too long (106 > 79 characters)

Comment last updated at 2020-10-09 13:58:04 UTC

@pllim
pllim requested a review from eteq August 27, 2020 19:51
@pllim pllim mentioned this pull request Aug 27, 2020
Merged
@pllim

pllim commented Aug 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

We can probably ignore pep8speak here. Not sure what to do about the change log complain. Please advise. (Oh, okay, I know now. Nvm)

I also slapped a "Manual Backport" label here, but if I am misusing that label, feel free to remove.

@aarchiba , if you can look over the diff and make sure I didn't mess things up, it would be greatly appreciated. Thanks!

@pllim
pllim force-pushed the backport-pr10437 branch from 9a0efbe to ca679a4 Compare August 27, 2020 20:01
@pllim

pllim commented Aug 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI failures:

p.s. I eyeballed the diff and nothing obviously wrong jumped out. We won't know for sure until the full CI can run. Currently, as of writing this, none of the failures are related.

aarchiba
aarchiba reviewed Aug 28, 2020
View reviewed changes

@aarchiba aarchiba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The diff looks pretty good - if the test suite passes it's just a question of double-checking for API changes, and I don't see any important ones.

Comment thread astropy/utils/data.py

def download_file(remote_url, cache=False, show_progress=True, timeout=None,
sources=None, pkgname='astropy', http_headers=None,
ftp_tls=False):

@aarchiba aarchiba Aug 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess the ftp_tls stuff made it into the LTS candidate but not a release? (So it's safe to take it back out before anyone notices?)

@pllim pllim Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, as long as this is the same milestone as #9964 , we should be okay.

@pllim pllim Aug 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the review, @aarchiba !

@pllim
pllim force-pushed the backport-pr10437 branch from ca679a4 to 525468b Compare October 6, 2020 23:30
@pllim

pllim commented Oct 7, 2020

Copy link
Copy Markdown
Member Author

html-docs and Travis CI doctest failure don't look related.

As for the parallel test, pretty sure it is due to pytest-dev/pytest-xdist#460 but not sure what is pulling in execnet. I see the same failure on v4.0.x branch too. I don't see execnet being pulled in on master, so something happened in getting rid of astropy-helpers and/or switching to tox that fixed it?

@astrofrog

astrofrog commented Oct 9, 2020

Copy link
Copy Markdown
Member

@pllim - can you rebase now that the CI should pass? (following #10810)

pllim and others added 2 commits October 9, 2020 09:57
@pllim
pllim force-pushed the backport-pr10437 branch from 525468b to b1c4138 Compare October 9, 2020 13:57
@pllim

pllim commented Oct 9, 2020

Copy link
Copy Markdown
Member Author

Rebased (again).

@astrofrog
astrofrog merged commit 4816dd8 into astropy:v4.0.x Oct 9, 2020
@astrofrog

astrofrog commented Oct 9, 2020

Copy link
Copy Markdown
Member

Thanks @pllim and @aarchiba!

@pllim
pllim deleted the backport-pr10437 branch October 9, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eteq eteq Awaiting requested review from eteq
1 more reviewer
@aarchiba aarchiba aarchiba left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug 🔥 Critical Manual Backport Refactoring utils.iers utils

Projects

None yet

Milestone

v4.0.2

Development

Successfully merging this pull request may close these issues.

4 participants

@pllim @pep8speaks @astrofrog @aarchiba