Fix #4886 look for a tarball first fallback to individual files #4904

samaloney · 2021-01-25T09:47:06Z

Description

Fixes #4886 by checking for a yearly tarball first if not found, try individual SRS files.

sunpy/net/dataretriever/sources/tests/test_noaa.py

sunpy/util/scraper.py

sunpy/net/dataretriever/sources/noaa.py

changelog/4904.bugfix.rst

sunpy/net/dataretriever/sources/noaa.py

samaloney · 2021-02-03T17:45:52Z

I expect the test_srs_current_year to fail as the SRS files for the first 5 day of 2021 are malformed

ftp> ls
229 Entering Extended Passive Mode (|||49316|)
150 Opening ASCII mode data connection for file list
-rw-r--r--   1 ftp      ftp           643 Jan  4 17:37 0101SRS.txt
-rw-r--r--   1 ftp      ftp           643 Jan  4 17:37 0102SRS.txt
-rw-r--r--   1 ftp      ftp           644 Jan  4 17:37 0103SRS.txt
-rw-r--r--   1 ftp      ftp           577 Jan  4 17:37 0104SRS.txt
-rw-r--r--   1 ftp      ftp           577 Jan  5 00:30 20210105SRS.txt
-rw-r--r--   1 ftp      ftp           563 Jan  6 00:30 20210106SRS.txt

nabobalis · 2021-02-14T12:34:33Z

I expect the test_srs_current_year to fail as the SRS files for the first 5 day of 2021 are malformed

ftp> ls
229 Entering Extended Passive Mode (|||49316|)
150 Opening ASCII mode data connection for file list
-rw-r--r--   1 ftp      ftp           643 Jan  4 17:37 0101SRS.txt
-rw-r--r--   1 ftp      ftp           643 Jan  4 17:37 0102SRS.txt
-rw-r--r--   1 ftp      ftp           644 Jan  4 17:37 0103SRS.txt
-rw-r--r--   1 ftp      ftp           577 Jan  4 17:37 0104SRS.txt
-rw-r--r--   1 ftp      ftp           577 Jan  5 00:30 20210105SRS.txt
-rw-r--r--   1 ftp      ftp           563 Jan  6 00:30 20210106SRS.txt

Can we get this fixed?

Cadair

Thanks a lot for this, it looks good. Does this fix the representation of the results in the table, or does it not have any affect on that?

I think there is one issue with your logic, but otherwise looks good.

Cadair · 2021-02-16T09:51:27Z

sunpy/net/dataretriever/sources/noaa.py

+        req_end_year = timerange.end.datetime.year
+        start_year = req_start_year if self.MIN_YEAR < req_start_year <= cur_year else None
+        end_year = req_end_year if self.MIN_YEAR < req_end_year <= cur_year else None
+        if start_year is None and end_year is None:


If the start time or end time is out of range, but not both, then the start_year or end_year variables would be None and would presumably fail later in this method? Do we need to clip them to the MIN_YEAR / MAX_YEAR?

Yea that didn't catch that kind of edge case reworked and added new test for these cases.

…/samaloney/sunpy into bugfix-srsclient-missing-tarball * 'bugfix-srsclient-missing-tarball' of https://github.com/samaloney/sunpy: (62 commits) changed test and updated changelog More scraper range fixes Review updates and more test fixes Fix scraper to pass tests Add changelog Update scraper to use relativedelta not units Fix sunpy#4886 look for a tarball first fallback to individual files Docs for writing new Fido clients (sunpy#4387) Changed the X-axis labels for the histograms Update sunpy/map/mapbase.py Added an alternate histogram and colormap shading to quicklook Assorted Doc Build Fixes (sunpy#4993) Reduce number of rotatedsun test examples (sunpy#4990) Reduce runtimes of some more tests (sunpy#4991) Update changelog/4881.bugfix.rst Update sunpy/map/mapbase.py Handle time scale FITS metadata in map.date Bump minimum required Matplotlib version Convert stale bot to a GitHub action (sunpy#4980) added repr line limit to fido ...

setup.cfg

sunpy/net/dataretriever/sources/noaa.py

Co-authored-by: Nabil Freij <nabil.freij@gmail.com>

Cadair

Wow this really spiralled didn't it.

I think it looks good, just a couple of small style comments. Has this grown to the point where it's no longer a simple bug fix and is really a behaviour change? (i.e. should this still get a 2.0 backport?)

sunpy/util/scraper.py

Cadair · 2021-02-17T15:33:51Z

sunpy/util/scraper.py

+
+        """
+        date_parts = [int(p) for p in date.strftime('%Y,%m,%d,%H,%M,%S').split(',')]
+        date_parts[-1] = date_parts[-1] % 60


So what's the damage with regards to leap seconds here. I assume it doesn't really matter, but what if someone has a URL with a 61st second in a minute in it?

I assume it doesn't work? It would get a different file?

Yea if you are actually looking for a file with a 60th second you won't find it even if it was there. I think that's a pretty extreme edge case and I don't even think it would have worked before either.

sunpy/net/dataretriever/sources/noaa.py

sunpy/util/scraper.py

Co-authored-by: Stuart Mumford <stuart@cadair.com>

sunpy-backport · 2021-02-18T10:00:10Z

The backport to 2.0 failed:

Commits ["c329146f8241a5bdca30d62522b1f26bd3d1f409","95ff4528f90306e136cc7ec3287bf6a4ba39180a","66513bf27202ab70307ce811047aed3631369a9d","f5d039457875e1c4678c39d84f5d0ab029fb5b41","9ada04fda2f5e522f8619a9a82af1c1e9c7c3880","4bd556a35a78f1e4f294504a5ab85429589b6605","83d8f4b32f25883abb4ca13069600995b4b57490","5fea615af9720935c3c45e9c4ecb7f7d06677590","4d72bcf769196fd987314a93b698262579f3c155","8dbe4b69cbfee5e11034ad92474fccc18c4d929e","703f56e85857f45c4624913a335258dcbec9a78f","3d210cfdf1a629de8519dbab6ab244c168952aa5","2f3d1fd72beab4c35d40fbe5ec3f5570ed09d31c","0ed1bf3052970bc4ff2afdbaee3abf57d76e436b","163d1ec4248eb619eb9e28911050d799f1c40550","47f025520f1613c930bca9b8022849cb263276b8","381a120518b81af1d73e770461630b7e93aaca2b"] could not be cherry-picked on top of 2.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 2.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 c329146f8241a5bdca30d62522b1f26bd3d1f409 95ff4528f90306e136cc7ec3287bf6a4ba39180a 66513bf27202ab70307ce811047aed3631369a9d f5d039457875e1c4678c39d84f5d0ab029fb5b41 9ada04fda2f5e522f8619a9a82af1c1e9c7c3880 4bd556a35a78f1e4f294504a5ab85429589b6605 83d8f4b32f25883abb4ca13069600995b4b57490 5fea615af9720935c3c45e9c4ecb7f7d06677590 4d72bcf769196fd987314a93b698262579f3c155 8dbe4b69cbfee5e11034ad92474fccc18c4d929e 703f56e85857f45c4624913a335258dcbec9a78f 3d210cfdf1a629de8519dbab6ab244c168952aa5 2f3d1fd72beab4c35d40fbe5ec3f5570ed09d31c 0ed1bf3052970bc4ff2afdbaee3abf57d76e436b 163d1ec4248eb619eb9e28911050d799f1c40550 47f025520f1613c930bca9b8022849cb263276b8 381a120518b81af1d73e770461630b7e93aaca2b
# Create a new branch with these backported commits.
git checkout -b backport-4904-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4904-to-2.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 2.0 and the compare/head branch is backport-4904-to-2.0.

sunpy-backport · 2021-02-18T10:01:35Z

The backport to 2.1 failed:

Commits ["c329146f8241a5bdca30d62522b1f26bd3d1f409","95ff4528f90306e136cc7ec3287bf6a4ba39180a","66513bf27202ab70307ce811047aed3631369a9d","f5d039457875e1c4678c39d84f5d0ab029fb5b41","9ada04fda2f5e522f8619a9a82af1c1e9c7c3880","4bd556a35a78f1e4f294504a5ab85429589b6605","83d8f4b32f25883abb4ca13069600995b4b57490","5fea615af9720935c3c45e9c4ecb7f7d06677590","4d72bcf769196fd987314a93b698262579f3c155","8dbe4b69cbfee5e11034ad92474fccc18c4d929e","703f56e85857f45c4624913a335258dcbec9a78f","3d210cfdf1a629de8519dbab6ab244c168952aa5","2f3d1fd72beab4c35d40fbe5ec3f5570ed09d31c","0ed1bf3052970bc4ff2afdbaee3abf57d76e436b","163d1ec4248eb619eb9e28911050d799f1c40550","47f025520f1613c930bca9b8022849cb263276b8","381a120518b81af1d73e770461630b7e93aaca2b"] could not be cherry-picked on top of 2.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 2.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 c329146f8241a5bdca30d62522b1f26bd3d1f409 95ff4528f90306e136cc7ec3287bf6a4ba39180a 66513bf27202ab70307ce811047aed3631369a9d f5d039457875e1c4678c39d84f5d0ab029fb5b41 9ada04fda2f5e522f8619a9a82af1c1e9c7c3880 4bd556a35a78f1e4f294504a5ab85429589b6605 83d8f4b32f25883abb4ca13069600995b4b57490 5fea615af9720935c3c45e9c4ecb7f7d06677590 4d72bcf769196fd987314a93b698262579f3c155 8dbe4b69cbfee5e11034ad92474fccc18c4d929e 703f56e85857f45c4624913a335258dcbec9a78f 3d210cfdf1a629de8519dbab6ab244c168952aa5 2f3d1fd72beab4c35d40fbe5ec3f5570ed09d31c 0ed1bf3052970bc4ff2afdbaee3abf57d76e436b 163d1ec4248eb619eb9e28911050d799f1c40550 47f025520f1613c930bca9b8022849cb263276b8 381a120518b81af1d73e770461630b7e93aaca2b
# Create a new branch with these backported commits.
git checkout -b backport-4904-to-2.1
# Push it to GitHub.
git push --set-upstream origin backport-4904-to-2.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 2.1 and the compare/head branch is backport-4904-to-2.1.

…unpy#4904) * Fix sunpy#4886 look for a tarball first fallback to individual files * Update scraper to use relativedelta not units * Add changelog * Fix scraper to pass tests * Review updates and more test fixes * More scraper range fixes * Fix sunpy#4886 look for a tarball first fallback to individual files * Update scraper to use relativedelta not units * Add changelog * Fix scraper to pass tests * Review updates and more test fixes * More scraper range fixes * changed test and updated changelog * Rework date logic and update gong doc test. * Update sunpy/net/dataretriever/sources/noaa.py Co-authored-by: Nabil Freij <nabil.freij@gmail.com> * Apply suggestions from code review Co-authored-by: Stuart Mumford <stuart@cadair.com> Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Stuart Mumford <stuart@cadair.com>

[Backport 2.1] Fix #4886 look for a tarball first fallback to individual files (#4904)

…#4904) (#5013) Co-authored-by: Nabil Freij <nabil.freij@gmail.com> Co-authored-by: Stuart Mumford <stuart@cadair.com> Co-authored-by: Shane Maloney <shane.maloney@tcd.ie>

nabobalis reviewed Jan 28, 2021

View reviewed changes

sunpy/net/dataretriever/sources/tests/test_noaa.py Outdated Show resolved Hide resolved

nabobalis added [BugFix] net Affects the net submodule labels Jan 28, 2021

samaloney commented Jan 29, 2021

View reviewed changes

sunpy/util/scraper.py Show resolved Hide resolved

samaloney commented Jan 29, 2021

View reviewed changes

sunpy/net/dataretriever/sources/noaa.py Outdated Show resolved Hide resolved

Shane Maloney added 2 commits January 29, 2021 10:09

Fix sunpy#4886 look for a tarball first fallback to individual files

c329146

Update scraper to use relativedelta not units

95ff452

samaloney force-pushed the bugfix-srsclient-missing-tarball branch from f67610e to 656f8fa Compare January 29, 2021 10:13

samaloney marked this pull request as ready for review January 29, 2021 10:13

samaloney requested a review from a team as a code owner January 29, 2021 10:13

Cadair requested changes Jan 29, 2021

View reviewed changes

Shane Maloney added 2 commits January 29, 2021 10:27

Add changelog

66513bf

Fix scraper to pass tests

f5d0394

samaloney force-pushed the bugfix-srsclient-missing-tarball branch from 0da4426 to f5d0394 Compare January 29, 2021 12:45

Review updates and more test fixes

9ada04f

samaloney mentioned this pull request Feb 3, 2021

SRSClient results have the wrong timestamp #4922

Closed

More scraper range fixes

4bd556a

nabobalis force-pushed the bugfix-srsclient-missing-tarball branch 3 times, most recently from 1e8fdd5 to 78812bd Compare February 14, 2021 13:19

nabobalis requested a review from Cadair February 14, 2021 13:45

Shane Maloney added 5 commits February 16, 2021 09:47

Fix sunpy#4886 look for a tarball first fallback to individual files

83d8f4b

Update scraper to use relativedelta not units

5fea615

Add changelog

4d72bcf

Fix scraper to pass tests

8dbe4b6

Review updates and more test fixes

703f56e

Shane Maloney and others added 2 commits February 16, 2021 09:48

More scraper range fixes

3d210cf

changed test and updated changelog

2f3d1fd

Cadair force-pushed the bugfix-srsclient-missing-tarball branch from 78812bd to 2f3d1fd Compare February 16, 2021 09:48

Cadair requested changes Feb 16, 2021

View reviewed changes

nabobalis reviewed Feb 17, 2021

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

nabobalis reviewed Feb 17, 2021

View reviewed changes

sunpy/net/dataretriever/sources/noaa.py Outdated Show resolved Hide resolved

Rework date logic and update gong doc test.

163d1ec

samaloney force-pushed the bugfix-srsclient-missing-tarball branch from 17e13eb to 163d1ec Compare February 17, 2021 12:45

Update sunpy/net/dataretriever/sources/noaa.py

47f0255

Co-authored-by: Nabil Freij <nabil.freij@gmail.com>

samaloney force-pushed the bugfix-srsclient-missing-tarball branch from da7d437 to 47f0255 Compare February 17, 2021 13:36

Cadair reviewed Feb 17, 2021

View reviewed changes

Cadair added this to Release blockers in 2.1 Feb 17, 2021

Cadair reviewed Feb 17, 2021

View reviewed changes

sunpy/util/scraper.py Outdated Show resolved Hide resolved

Apply suggestions from code review

381a120

Co-authored-by: Stuart Mumford <stuart@cadair.com>

samaloney force-pushed the bugfix-srsclient-missing-tarball branch from ab5fc6b to 381a120 Compare February 17, 2021 20:21

samaloney mentioned this pull request Feb 17, 2021

Scraper Redesign #4888

Open

nabobalis approved these changes Feb 17, 2021

View reviewed changes

2.1 automation moved this from Release blockers to Reviewer approved Feb 18, 2021

Cadair approved these changes Feb 18, 2021

View reviewed changes

Cadair merged commit 6685160 into sunpy:master Feb 18, 2021

2.1 automation moved this from Reviewer approved to Done Feb 18, 2021

nabobalis added the Still Needs Manual Backport This PR needs manually backporting label Feb 18, 2021

Cadair added a commit that referenced this pull request Feb 18, 2021

Merge pull request #5011 from Cadair/2.1

13b8392

[Backport 2.1] Fix #4886 look for a tarball first fallback to individual files (#4904)

nabobalis removed the Still Needs Manual Backport This PR needs manually backporting label Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4886 look for a tarball first fallback to individual files #4904

Fix #4886 look for a tarball first fallback to individual files #4904

samaloney commented Jan 25, 2021

samaloney commented Feb 3, 2021

nabobalis commented Feb 14, 2021

Cadair left a comment

Cadair Feb 16, 2021

samaloney Feb 17, 2021

Cadair left a comment

Cadair Feb 17, 2021

nabobalis Feb 17, 2021 •

edited

samaloney Feb 18, 2021 •

edited

sunpy-backport bot commented Feb 18, 2021

sunpy-backport bot commented Feb 18, 2021

Fix #4886 look for a tarball first fallback to individual files #4904

Fix #4886 look for a tarball first fallback to individual files #4904

Conversation

samaloney commented Jan 25, 2021

Description

samaloney commented Feb 3, 2021

nabobalis commented Feb 14, 2021

Cadair left a comment

Choose a reason for hiding this comment

Cadair Feb 16, 2021

Choose a reason for hiding this comment

samaloney Feb 17, 2021

Choose a reason for hiding this comment

Cadair left a comment

Choose a reason for hiding this comment

Cadair Feb 17, 2021

Choose a reason for hiding this comment

nabobalis Feb 17, 2021 • edited

Choose a reason for hiding this comment

samaloney Feb 18, 2021 • edited

Choose a reason for hiding this comment

sunpy-backport bot commented Feb 18, 2021

sunpy-backport bot commented Feb 18, 2021

nabobalis Feb 17, 2021 •

edited

samaloney Feb 18, 2021 •

edited