Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
dataset.services()
method to list available services #500Add
dataset.services()
method to list available services #500Changes from 23 commits
3c4d293
78a6fcb
285b0d6
5a40e8a
8088690
911b954
bc9c2f4
0bb9f11
692d3cf
9e37fe2
e8dfc74
5404df3
1c41659
daf1bb4
07771e1
240c930
4a710f4
6cf0f64
20d395d
44917d9
71973dc
16512d1
057d7fd
41d18b8
14dc3ef
64caf3d
f67365b
0c66c66
9dfa385
cc24470
2362eb8
d1bebe3
0e505ae
b437341
8ecacd1
b874c01
61b2e04
49fb87d
c2af6b3
9474b33
f6d3776
c0a3c8a
79319c8
01bad9c
2c29d73
8dd2402
a1b64f2
d99dc3c
134a7b7
783d996
ba0eb8f
39f8963
86ab7a8
3971454
39b86a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we getting only the item at index 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the results response, the data always seems to be returned under a list of one element which contains all of the metadata. In order to provide some filtering, I chose only to return the provider_id and the UMM JSON response for each service. See attached sample-results-response.json.
It also looks like this is the case in the CMR API documentation but to be on the safe side I will figure out how to iterate over the list to make sure we don't miss anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you address my suggestion I just added to the
get
method, you should not need to usejson.loads
because theget
method will returned parsed results.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chuckwondo - I can update the code to iterate over the "items" list in the CMR service query response but then I end up returning a list of lists to the end user. See attached parsed.json. What do you think of returning a list of lists that contains the items?
Here is an example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there meaning to the nested list structure? If not, we could use
itertools.chain()
to flatten it out, IIRC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #500 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this is not in
search.py
, likeDataCollections
andDataGranules
are?I'm not opposed to keeping this in a separate file, but splitting each type of query class into their own modules might perhaps be left as a task for a separate issue (after discussing whether or not we want to do so).
For consistency with existing code, I suggest moving this to
search.py
and also renaming it to be pluralized:DataServices
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had initially wanted to place
DataService
insearch.py
but because the results module uses theDataService
class to query and parse results, a circular dependency is created between the search and results modules. I also thought theDataService
class might grow as we add in the plugin architecture and could serve as the main entrypoint for loading and accessing plugins.I can look into re-architecting the code so that there is a
DataServices
class insearch.py
andDataService
class inresults.py
to be more consistent with the Collections and Granules structure. I was initially thinking that the services would be returned for a collection rather than having a separatesearch_services
function.RIght now the end user can query services like this:
Making it pretty easy to return service data for a collection. If I re-architect as mentioned above. The user would search a service like this:
I like how easy it is to return service data in the first code snippet but also want to make sure we are building a codebase that is consistent and easy to modify (add to) in the future as I think we want to build off of this with the plugin architecture.
Open to suggestions and/or discussing at the next hackday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see the circular dependency you're referring to.
I still suggest your rename
DataService
(singular) toDataServices
(plural) making it consistent withDataCollections
andDataGranules
.Regarding the "easier" code you mention above, I agree, but that doesn't preclude providing a
search_services
method as well. Users can then do both things: (a) calldataset.services()
to get the services associated with a dataset, or (b) callsearch_services
to search for services more generally, not necessarily specific to a dataset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider these separate features and follow-up later to add even more convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. In fact, I suggest that
dataset.services()
simply invokesearch_services
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me. My only request is to not name anything "utils." It's a pet peeve of mine because it's such a generic name, as to have no meaning. Happy to iron out kinks with you at the next hack day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this and feel so called out 🤣 I'm very prone to creating a
utils
subpackage but I always regret it later and am trying to get better at it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree on
utils
😄 but it does look like there is already autils
directory. Should we consider moving that to a different name? Or maybe I am misunderstanding!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I hadn't noticed that there's already a
utils
. Oh well. We can worry about that another time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that was me, sorry 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code already exists in
ServiceQuery
, so simply call the superclass method. Ideally, this method doesn't need to be here at all, but for now we do this simply for generating docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be a call to super like this:
super().get(limit)
. Will test and implement in code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hang on. We want
umm_json
, notjson
, so we do need to implement this here becausepython_cmr
currently parses only when the format isjson
, notumm_json
.However, we already implement this generally in
search.get_results
, so this should likely be something like so:However, this will need you to tweak
search.py
as well, as follows:First, change
from cmr import CollectionQuery, GranuleQuery
tofrom cmr import CollectionQuery, GranuleQuery, ServiceQuery
Next, change this:
to this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I make these changes I get a circular dependency as services.py tries to import from
.search
whileresults.py
is trying to importDataCollection, DataGranule
from.search
. The results module usesDataService
to query and parse results. Also see this comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, yeah, I see the circularity. We may need to rethink where to put things to avoid circularities.