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

Fix CAMS message error handler #1905

Merged
merged 14 commits into from Dec 12, 2023
Merged

Fix CAMS message error handler #1905

merged 14 commits into from Dec 12, 2023

Conversation

GillesFischerV
Copy link
Contributor

@GillesFischerV GillesFischerV commented Nov 7, 2023

  • Closes CAMS update: extended coverage and new error message handling #1799
  • I am familiar with the contributing guidelines
  • Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Hi! I am Gilles Fischer, a new member of SoDa team working on CAMS project.

With this commit, I suggest a fix to the message error handler for cams request.
If an error occurs on our side, we will return status 400 - Bad request. As it lacks any details, I suggest to use the error details also available in our response to facilitate the user experience.
It is the default behavior in my PR but I could be only available as a verbose option in get_cams.

It is my first time working on a open source project so hopefully my contribution will meet your expectation and needs.

Let me know if anything required more details or work!

@AdamRJensen AdamRJensen added io remote-data triggers --remote-data pytests labels Nov 7, 2023
@AdamRJensen AdamRJensen added this to the v0.10.3 milestone Nov 7, 2023
@AdamRJensen
Copy link
Member

@GillesFischerV thank you very much for the contribution!

This is definitely a nice enhancement of the CAMS API and simplifies the code on our end 😄 However, the error messages returned are still not great, e.g., when requesting a latitude of 1000 I get the error message below. Anyhow, that's not to do with this PR, but something I wanted to bring up for the CAMS team to consider.

Process error: bash: line 1: 1789700 Segmentation fault core dumped /mnt/soda-prod/soda_production/helioclim/bin/compute_gc_mcclear -m sg2 -o /tmp/mcclear_000000004cf07540 -f csv -s PT01H -t UT /mnt/soda-cache/McClear/hcday_builder_bbox_msg_mcclear_TSV.conf 1000.000000 9.501800 -999.000000 2022-01-01 2022-01-01

To do
It would be great if you could add an entry to that WhatsNew file and add your name as well.

Additionally, there's currently one remote test failing that needs to be modified (pvlib/tests/iotools/test_sodapro.py::test_get_cams_bad_request) - if you need help let me know.

@GillesFischerV
Copy link
Contributor Author

GillesFischerV commented Nov 8, 2023

@AdamRJensen Thanks for your feedback !

I had apparently an issue on my side where I mistook the result of the Unit test. Working on it right now.
Will also add an entry on WhatsNew as you asked.

I will look into the error messages returned with an altitude of 1000 to see if we can fix it.
Your message seems to implied that other use case have similar issues, is there other recurrent one that you would like to highlight ?

@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels Nov 21, 2023
@AdamRJensen
Copy link
Member

Your message seems to implied that other use case have similar issues, is there other recurrent one that you would like to highlight ?

When specifying an email that is not associated with a registered user the error message is also not helpful.

I've taken the liberty to also update the geographical coverage description (now that you use Himawari) - please ensure that it is correct.

Thanks again for this PR :)

@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels Nov 21, 2023
@GillesFischerV
Copy link
Contributor Author

GillesFischerV commented Nov 22, 2023

Your message seems to implied that other use case have similar issues, is there other recurrent one that you would like to highlight ?

When specifying an email that is not associated with a registered user the error message is also not helpful.

I've taken the liberty to also update the geographical coverage description (now that you use Himawari) - please ensure that it is correct.

Thanks again for this PR :)

I noticed the lacking error message in case of not registered email, planning to fix it for next production release.

Your change on coverage description is OK.

You're welcome, glad to contribute to PVlib!

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @GillesFischerV! I have only one comment about the mock test.

@@ -248,20 +248,21 @@ def test_get_cams_bad_request(requests_mock):
requests inputs. Also tests if the specified server url gets used"""

# Subset of an xml file returned for errornous requests
mock_response_bad = """<?xml version="1.0" encoding="utf-8"?>
mock_response_bad_text = """<?xml version="1.0" encoding="utf-8"?>
<ows:Exception exceptionCode="NoApplicableCode" locator="None">
<ows:ExceptionText>Failed to execute WPS process [get_mcclear]:
Please, register yourself at www.soda-pro.com
</ows:ExceptionText>"""
Copy link
Member

Choose a reason for hiding this comment

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

I think this error text is out of date. From the real API, I get this response when supplying an unregistered email:

<?xml version="1.0" encoding="UTF-8"?>\n<!-- PyWPS 4.5.0 -->\n<ows:ExceptionReport xmlns:ows="http://www.opengis.net/ows/1.1" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.opengis.net/ows/1.1 http://schemas.opengis.net/ows/1.1.0/owsExceptionReport.xsd" version="1.0.0">\n <ows:Exception exceptionCode="NoApplicableCode" locator="" >\n <ows:ExceptionText>Process error: Dear User, thank you for your interest in the McClear service provided by MINES ParisTech. The service has been developed with funding from the European Union s Copernicus Programme CAMS project . The data policy imposed by the European Union in CAMS project is that users must be identified. Please,</ows:ExceptionText>\n </ows:Exception>\n</ows:ExceptionReport>

Probably we should update to the current API message, right?

Note also that the end of the message seems to be truncated ("Please, " do what?).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see this error message has already been discussed :)

If it will change again soon, what should we do here? I don't think it makes sense to test on out of date messages, but it seems that even the current message will soon be out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the message will be changed.
As far as I understand, because we handle the answer through mock, the real content of the message is not important in the test as long as the format is correctly simulated.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it is not critical that the text be an exact match, but we should still try to mimic the real API's behavior to whatever extent is possible. In this case, maybe the best we can do is to add a comment in the test saying that this text isn't an exact match.

pvlib/iotools/sodapro.py Outdated Show resolved Hide resolved
@cwhanse cwhanse mentioned this pull request Dec 8, 2023
15 tasks
AdamRJensen and others added 2 commits December 12, 2023 15:57
Co-authored-by: Anton Driesse <anton.driesse@pvperformancelabs.com>
@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels Dec 12, 2023
@AdamRJensen
Copy link
Member

@kandersolar This just passed all tests including the remote tests - just pushed some inconsequential flake8 fixes - should be ready to merge.

@kandersolar
Copy link
Member

Thanks @GillesFischerV for the PR and @AdamRJensen and @adriesse for the reviews!

@GillesFischerV, we hope you stick around and continue contributing :)

@kandersolar kandersolar merged commit f430a74 into pvlib:main Dec 12, 2023
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CAMS update: extended coverage and new error message handling
4 participants