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

use Cmr-search-after instead of paging #87

Merged
merged 51 commits into from
Feb 9, 2022
Merged

Conversation

wallinb
Copy link
Contributor

@wallinb wallinb commented Jun 18, 2020

Unfortunately EGI does not support scrolling yet, so this only updates granule search.

Some thoughts: Ordering and querying could be less coupled than they are now e.g. place_order calls get_avail to know how many granules will be ordered, but the sub-setting service will do the similar search itself to fulfill the order. In some edge cases these two searches can become inconsistent with each other, especially now that one uses scrolling and one uses paging. At the moment it is the only way to know/estimate the number of pages/orders necessary so I left it as is. Maybe long term we want to separate 'querying' functionality from 'ordering' into separate classes or something?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #87 (02481ae) into development (a9c7813) will increase coverage by 0.00%.
The diff coverage is 52.63%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development      #87   +/-   ##
============================================
  Coverage        55.45%   55.45%           
============================================
  Files               28       28           
  Lines             1951     1960    +9     
  Branches           404      406    +2     
============================================
+ Hits              1082     1087    +5     
- Misses             800      804    +4     
  Partials            69       69           
Impacted Files Coverage Δ
icepyx/core/APIformatting.py 75.64% <ø> (-0.16%) ⬇️
icepyx/core/query.py 51.76% <ø> (ø)
icepyx/tests/is2class_query.py 21.73% <0.00%> (ø)
icepyx/core/granules.py 38.42% <52.94%> (+1.11%) ⬆️
icepyx/tests/test_granules.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9c7813...02481ae. Read the comment docs.

@JessicaS11 JessicaS11 linked an issue Feb 2, 2022 that may be closed by this pull request
@JessicaS11 JessicaS11 changed the title Cmr scrolling search use Cmr-search-after instead of paging Feb 2, 2022
@JessicaS11
Copy link
Member

See #22 (comment) for an update on why the switch from cmr scrolling to cmr-search-after.

Note: the PR-linked RTDs build is failing because of a recent pip release combined with the fact that you can't wipe the build environment for PRs. A branch build shows they pass with no issues.

@JessicaS11
Copy link
Member

@betolink Do you think you could review this PR (or recommend someone who can)?

@betolink
Copy link
Contributor

betolink commented Feb 3, 2022

Hi @JessicaS11 I can take a look this week, maybe Andy would be another person to ask as well. One thing, Search-After is great for performance on the CMR side but it is not compatible with deep pagination.

i.e. this would be an anti-pattern for an initial search:

reqparams = {'page_size': 2000, 'page_num': 3}

Are the pagination parameters exposed to the users or it's an internal Icepyx interface? As a user I don't really need to access say page 565, usually we just want to know how many granules we get back from CMR and then download them all. The PR Looks good!

@JessicaS11
Copy link
Member

Are the pagination parameters exposed to the users or it's an internal Icepyx interface?

Primarily the latter. With this PR, page_num is not submitted during the CMR query step, instead using Search_After to collect available results. page_numIS used by icepyx during ordering (and download) to iterate through the orders, since to my understanding Search_After is not available outside of CMR queries. reqparams, and thus page_num, is not hidden to the user, but my guess is nobody really pays it any mind (and if they don't view reqparams after placing an order, they'll probably never see page_num since it's not a required parameter for the CMR query step).

In general, I tried to design icepyx so that users had 100% control over (and visibility of) any request/parameters being sent to an endpoint if they want it (and to help it map clearly to the CMR docs for programmatic data access), but don't actually have to be bothered with the parameter dictionaries and request syntax/parsing otherwise.

@JessicaS11 JessicaS11 dismissed their stale review February 3, 2022 16:47

This PR has been updated to use a cmr-search-after; this review is outdated.

@JessicaS11 JessicaS11 requested review from andypbarrett and removed request for liuzheng-arctic February 3, 2022 16:47
Copy link
Contributor

@betolink betolink left a comment

Choose a reason for hiding this comment

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

It's looking great, there won't be a noticeable performance difference for Icepyx but CMR will handle these big queries better than just using pagination. I'll wait for Andy to take a look before approving it. Great work!

Copy link
Contributor

@betolink betolink left a comment

Choose a reason for hiding this comment

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

I think this PR is ready to be merged and tested in the dev branch. Looks like regular pagination and search-after will coexist and will be transparent to the users so that's great.

@JessicaS11 JessicaS11 merged commit e424fed into development Feb 9, 2022
@JessicaS11 JessicaS11 deleted the cmr-scrolling-search branch February 9, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reqparams page_num gets overwritten when placing order interact with NSIDC API without paging
8 participants