-
Notifications
You must be signed in to change notification settings - Fork 84
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
#500
Conversation
dataset.services()
method to list available Harmony services
It seems like the existing integration tests are failing due to credentials which I had set up as environment variables and in a |
Hope you don't mind the rename, was finding the old name difficult to remember in my notifications :) |
@betolink @andypbarrett I think this may need documenting! As a developer, how do I run integration tests on my laptop? |
…ature/issue-447
…ccess into feature/issue-447
@mfisher87 - I think that I was able to fix the integration test and also modified the services unit and integration tests to use VCR. All unit tests are passing. I ran the integration tests locally and here is the summary: ==================== short test summary info ================================= FAILED tests/integration/test_auth.py::test_auth_can_read_from_netrc_file - AssertionError: False is not true ====== 7 failed, 71 passed, 1 skipped in 496.39s (0:08:16) ========================================== I wonder if these tests need to be run in the same AWS region as the data? Other than that I think this PR can be merged but let me know how that typically works for |
Will you be at hack day this coming week? I may not have time to review this before then 😬 |
@mfisher87 - Unfortunately I am on travel this coming week so I can't make it but I do plan to attend the next earthaccess hack day in a few weeks. We can discuss then if it's easier! 😄 |
Sounds like a good plan :) Safe and fun travels! |
@chuckwondo @betolink @mfisher87 - Any thoughts on the failing Integration Tests and the Python3.9 Unit Test? I implemented the fix recommended by @chuckwondo to decode the compressed VCR response and updated my fork with changes from It seems like the integration tests are failing with an Earthdata login? Maybe this is an issue introduced by merging main with my fork - How does the workflow get Earthdata credentials? INFO earthaccess.auth:auth.py:266 Authentication with Earthdata Login failed with:
{"error":"invalid_header","error_description":"Invalid header"}
...
File "/home/runner/work/earthaccess/earthaccess/tests/integration/test_cloud_download.py", line 67, in <module>
assertions.assertTrue(auth.authenticated) And the Unit Test may have to do with the version of Python and the requests library trying to deal with the decompression. FAILED tests/unit/test_services.py::TestServices::test_service_results - requests.exceptions.ContentDecodingError: ('Received response with content-encoding: gzip, but failed to decode it.', error('Error -3 while decompressing data: incorrect header check'))
FAILED tests/unit/test_services.py::TestServices::test_services - requests.exceptions.ContentDecodingError: ('Received response with content-encoding: gzip, but failed to decode it.', error('Error -3 while decompressing data: incorrect header check')) How long do plan to support Python3.9? I am wondering if it doesn't play nicely with the VCR I don't have a ton of time at the moment to continue to troubleshoot but hope to get back into the swing of things in the next month or so. |
You can ignore these integration test failures, which we've been seeing for other PRs as well:
@mfisher87 and I have been trying to determine why this happens, but for these particular integration test failures, we have seen that once PRs with such failures (and only such failures) are merged, they pass on the merged PRs. Regarding the unit test failurs for Python 3.9, I'll do some digging. It appears that we may need to do some Python (or urllib3/requests lib) version check to determine whether or not the vcrpy hint I provided should be applied, since the unit tests now fail only for Python 3.9. I'll follow up once I discover something. |
We thought at first that setting the needed secrets on the fork would enable the tests to pass, but nah. I'd like one really clear and definitive resource on how to make testing with forks more accessible. I'd love to contribute something like that to opensource.guide but at this point we have like 4 or 5 weird mysteries or user experience pain points to solve for 😆 |
@nikki-t, I managed to get all the unit tests across all python versions to pass locally. It took a bit of wrangling to subdue the issue with vcrpy and gzip compression, but I used your branch as the basis for sorting things out. However, it might be a bit cumbersome for us to have you apply my changes to your branch via comments on your PR, so let me construct a patch for you to apply to your branch. If you don't want to deal with applying a patch, I could open a new PR. |
@chuckwondo since Nikki has set this setting, what do you think about just pushing to this PR? Once you add the source repository as a remote, GH will let you push to this branch. |
Thanks @chuckwondo!! Let me know if you need me to apply the patch or if you are able to push to the PR like @mfisher87 mentions. |
@nikki-t and @mfisher87, this should do the trick. The failing int. tests are the usual suspects that should pass once merged. |
dataset.services()
method to list available Harmony servicesdataset.services()
method to list available services
@mfisher87, @nikki-t, do either of you want to take another look at this now that I've tweaked it to get all the unit tests to pass (and believe the int. tests will also pass once merged)? |
@chuckwondo - This looks good to me. I like the top-level function to search for services. |
@mfisher87, last chance for you to take another look before I merge |
Hey Chuck, thanks for reminding me! I am OK with merging based on what I've seen already! But won't have opportunity to do a real "Review" until hack time tomorrow 😬 |
No worries. Since this has been long-running, and we've all gone over it, I'm going to merge it so we can then focus on more recent PRs. |
Wonderful, thank you so much for helping keeping the PRs flowing, Chuck! And of course, Nikki, wonderful work! 🙇 |
Github Issue: 447
Description
List available Harmony services for a collection. As a first step to facilitating the use of services in earthaccess, earthaccess was modified so that it can list the available services for a collection.
Overview of work done
A new services module was created that performs service queries. The results module was updated to include a
services
function which will use the services module to query a collection's services and return theprovider-id
andumm
JSON for the service.Sample services results:
Overview of verification done
Tested new service functionality on four collections:
Overview of integration done
Created a new unit test to test DataService
get
. Unit test coverage:Create a new integration test that tests the services functionality from the service query to the parsing of query results. Integration test coverage:
PR checklist:
Pending:
📚 Documentation preview 📚: https://earthaccess--500.org.readthedocs.build/en/500/