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

Completing Luquillo CZO WOFpy endpoint integration into CUAHSI WS #219

Closed
emiliom opened this issue Nov 27, 2017 · 83 comments
Closed

Completing Luquillo CZO WOFpy endpoint integration into CUAHSI WS #219

emiliom opened this issue Nov 27, 2017 · 83 comments

Comments

@emiliom
Copy link
Member

emiliom commented Nov 27, 2017

@martinseul, I'm following up on our emails last week about our common goal to identify and fix the remaining issue(s) preventing complete functioning of @miguelcleon's Luquillo CZO WOFpy endpoint in the CUAHSI Water Services. The homepage to that WOFpy endpoint is at http://odm2admin.cuahsi.org/wofpy/odm2lczo/, unless @miguelcleon has changed it. It looks like the test network page at qa-hiscentral is http://qa-hiscentral.cuahsi.org/pub_network.aspx?n=100002, and the corresponding test service is at http://qa-hiscentral.cuahsi.org/testpage.aspx?n=100002

For reference, we've been discussing this for some time at WOFpy issue #196. The last comments were from @lsetiawan (Don) and I on Nov 15:

Based on conversation with Martin, The problem seems to lie on tags not being closed properly for empty attributes. For example <qualityControlLevelCode/> should be <qualityControlLevelCode></qualityControlLevelCode> for the xml to be validated.

and

I spoke with Martin a bit about this. What we're doing is not wrong (ie, <qualityControlLevelCode/> is not incorrect, as demonstrated by our schema validation), but it sounds like there's a client parsing expectation on their end.

As discussed via email last week, we'll use this issue to track progress. Thanks!

@miguelcleon
Copy link
Member

@emiliom yes the WOFpy endpoint homepage is still at http://odm2admin.cuahsi.org/wofpy/odm2lczo/

Is there an expectation that CUAHSI will change their parser or will @lsetiawan change the closing tags?

@emiliom
Copy link
Member Author

emiliom commented Nov 28, 2017

Is there an expectation that CUAHSI will change their parser or will @lsetiawan change the closing tags?

Possibly a combination of both? But we're definitely prepared to do the latter. I'm waiting to hear from @martinseul exactly what the issues are, though.

@miguelcleon
Copy link
Member

miguelcleon commented Nov 29, 2017

From Martin:

That is a fix that they will need to implement on their side, a fix on my side is not easy as the app relies on a .net xml parser that I don’t control.

I can run test once their XML is updated.

Sorry if that was not clear.

Miguel:

Ok, so just to confirm all tags need to be ended like <qualityControlLevelCode></qualityControlLevelCode>

From Martin:

Yes that’s the way it should be.

@lsetiawan
Copy link
Member

Thanks @miguelcleon. I am currently looking into changing the tags to close. It seems to be complex, but I'm working on it. I'll let you know as soon as I come up with a solution. Thanks.

@miguelcleon
Copy link
Member

@lsetiawan sounds good. Thank you!

@miguelcleon
Copy link
Member

@lsetiawan any luck with this?

@lsetiawan
Copy link
Member

@miguelcleon Not yet. I won't be able to work on this until tomorrow. Thanks.

@miguelcleon
Copy link
Member

ok

@lsetiawan
Copy link
Member

lsetiawan commented Dec 6, 2017

Alright, so after a lot of research. It seems like the problem lies on lxml library actually making empty value tags into a self closing tag. For example, if MethodLink is a blank string '', wofpy actually creates an xml tag <methodLink></methodLink>. However, this string gets turned into an lxml Element object by Spyne. In this object, the empty value tags turned into a self-closing tag <methodLink/>. I tried this manually:

>>> from lxml import etree
>>> xmlstring = '<methodLink></methodLink>'
>>> root = etree.fromstring(xmlstring)
>>> etree.tostring(root)
'<methodLink/>'

Right now one solution that I found to get the full '' is to replace '' with a space ' '. This in turn will make <methodLink> </methodLink>. Not sure if this is an okay solution. I need your input. Thanks!

@emiliom
Copy link
Member Author

emiliom commented Dec 6, 2017

Thank you, @lsetiawan. I won't be able to help or comment on this until after Christmas. @martinseul and @horsburgh, can you provide input on Don's question/suggestion? Also, Don, if you can reach out to Python lxml/etree experts to see if you can find an ideal solution to this, that'd be great.

@miguelcleon
Copy link
Member

I'll email @martinseul , it's possible the space might cause a problem, but maybe not.

@miguelcleon
Copy link
Member

@lsetiawan are there cases where a self closing tag is created where a number might be expected? Or are these being generated only for charcter fields?

CUAHSIs parser might treat them different.

@lsetiawan
Copy link
Member

If a number is empty, the value getting passed is probably None therefore, these tags isn't even included. So i guess these are being generated only for character fields. Thanks.

@miguelcleon
Copy link
Member

Ok, I looked over some of the xml being generated by WOFpy and I didn't see any self closing tags for numbers. We can probably assume they aren't generated.

@horsburgh
Copy link
Member

This is something I've always wondered about in the implementation of WOFPy, the CUAHSI WaterOneFlow web services, and WaterML. I suspect that David Valentine may have used a convention for this in his original development of WaterOneFlow, which formed the basis for the services that CUAHSI HIS uses. For the WaterOneFlow services I am hosting, I am using the last version of code that Valentine wrote before the end of the HIS project, and they all seem to work with both HIS Central and the WaterML R package, so the problem seems to be specific to WOFPy.

Since WOFPy is a different code base using different tools, the convention/assumption may not be the same. There are potentially three conventions, right?:

  1. - opening and closing tags with no value
  2. - self closing tag
  3. Omitting the methodLink element when there is no value

I don't think the right solution is to put a space in between the tags in 1.

I think we have to get some info from @martinseul because all three of these should probably be valid given that methodLink is not a required attribute. However, if there is a convention that the CUAHSI HIS Central is expecting, he's can tell us that, and we can either adopt that convention or try to talk them into making a change on their end.

@miguelcleon
Copy link
Member

@lsetiawan you can do this:

from lxml import etree
import lxml.html
xmlstring = '<methodLink></methodLink>'
root = etree.fromstring(xmlstring)
print(lxml.html.tostring(root))

<methodLink></methodLink>

Would that fix the problem?

@miguelcleon
Copy link
Member

miguelcleon commented Dec 6, 2017

@horsburgh Martin said he doesn't control the parser and can't make updates. I agree that having tags with spaces in between is not a great solution.

@lsetiawan
Copy link
Member

lsetiawan commented Dec 6, 2017

@miguelcleon Your solution would have to be implemented within Spyne itself which I do not have control over.

The example I gave was simply a demo, not something that is in WOFpy code.

@miguelcleon
Copy link
Member

@lsetiawan right, dang, we might have to go with adding the space. I emailed Martin.

@lsetiawan
Copy link
Member

I looked at the WaterML 1.0 GetSiteInfo Response, and it seems like there are some self-closing tags. This was able to be parsed by WaterML R. Though this is the REST response, I'll check and see what the actual SOAP response is.

<Source sourceID="64">
<Organization>LimnoTech</Organization>
<SourceDescription>Water Environment | Scientists Engineers</SourceDescription>
<Metadata/>
<SourceLink/>
</Source>

@lsetiawan
Copy link
Member

So, looking at the response SOAP XML via suds-jurko library. It seems like the tags are actually there. No self-closing.

<method methodID="2">                    
<methodCode>2</methodCode>                    
<methodLink></methodLink>               
</method>

Python code:

from suds.client import Client
client = Client('http://data.envirodiy.org/wofpy/soap/cuahsi_1_1/.wsdl')
response = client.service.GetSiteInfo(site='envirodiy:160065_Limno_Crossroads')

I guess the xml becomes self-closing when it is REST.

@miguelcleon
Copy link
Member

Maybe we want to test adding the space between the tags? you could just create a separate branch for that. After we test that, we can try to get CUAHSI to fix the parser.

@miguelcleon
Copy link
Member

unless @lsetiawan has another idea for what to do.

@miguelcleon
Copy link
Member

From Martin:

Having spaces is not an ideal solution but might work as a workaround for now. If you point me to a dev service I could test or you could update the service we already have and I could run some tests.
If we can isolate the problem to the tags than we can focus on this issue going forward. Could you also include a sample request for data values from wofpy there might be an additional problem forming the right request.

My Response:

Hi Martin,

Yeah I agree, it is not ideal to use the space. Yeah, my service is slightly behind, a manually applied all of the bug fixes though as we were testing them. I double checked with the release notes, looks like I’m just missing the much improved landing page.

But Anthony / Emilio do have an EnviroDIY WOFpy service which is completely up to date so it would be a good idea to try that http://data.envirodiy.org/wofpy/

A sample request for data:

http://data.envirodiy.org/wofpy/rest/1_1/GetValues?location=envirodiy:160065_Limno_Crossroads&variable=envirodiy:EnviroDIY_Mayfly_Temp&startDate=2017-01-01T00:00:00&endDate=2017-08-01T02:30:00

My service again is here: http://odm2admin.cuahsi.org/wofpy/odm2lczo/

Here is a sample request for data from my service:

http://odm2admin.cuahsi.org/wofpy/odm2lczo/rest/1_1/GetValues?location=odm2lczo:Rio%20Icacos%20Trib-IO&variable=odm2lczo:DO%20Concentration&startDate=2016-12-01T12:00:00&endDate=2016-12-01T14:30:00

@miguelcleon
Copy link
Member

@lsetiawan

From Martin:

Also most of our HIS infrastructure is still based on soap.
do you have a request body as well so I can test with fiddler?

@miguelcleon
Copy link
Member

@martinseul was able to register the EnviroDIY service on http://qa-hiswebclient.azurewebsites.net/

We need to check if the metadata, database fields in download and catalog and service statistics are correct, do additional testing and evaluate performance.

@lsetiawan can you take a look? you should select the envirodiy data service. Then you can select the time series like:

image

@miguelcleon
Copy link
Member

@lsetiawan the self closing tags are still a problem though. The EnviroDIY data doesn't include any fields that are empty, of those that end up in the XML at least. Can you confirm that? I've filled in the fields in my database that were empty so Martin can run the harvesting again and we can see if mine will work.

@lsetiawan
Copy link
Member

@miguelcleon Awesome! Yea. I just tested it. And it works! I was able to download the actual data just fine.

@lsetiawan the self closing tags are still a problem though. The EnviroDIY data doesn't include any fields that are empty, of those that end up in the XML at least. Can you confirm that?

I looked at the csv and xml for Beth_office temperature. I see that sourceLink is empty in xml, and it's filled with unknown within the csv. It seems to be working for me.

@miguelcleon
Copy link
Member

Huh, maybe it doesn't try to ingest that field or something.

@miguelcleon
Copy link
Member

@lsetiawan do the number of records match the total in your database? It probably should match the number of records in the timeseriesresultvalues table I would think.

@miguelcleon
Copy link
Member

I changed it back to https://dev-odm2admin.cuahsi.org/woflczo/

@emiliom
Copy link
Member Author

emiliom commented Feb 12, 2018

Testing on HIS central QA system isn't working so I'm in communication with @martinseul about that.

Darn. @miguelcleon, is this a new problem, or problems you had run into before and were unresolved? Or are you saying the HIS Central QA test system isn't working altogether (not just your endpoint)?

@miguelcleon
Copy link
Member

I'm not sure if it is a problem we ran into before. HIS central QA system (http://qa-hiscentral.cuahsi.org/testpage.aspx?n=100002) isn't retrieving siteInfo. I asked Martin for the SOAP envelope and I'm waiting to hear back.

@emiliom
Copy link
Member Author

emiliom commented Feb 12, 2018

I'm pretty sure you had gotten past retrieving siteInfo! But anyway ... fingers crossed.

@miguelcleon
Copy link
Member

miguelcleon commented Feb 14, 2018

From @martinseul

I checked he soap request and that fails with a 500 error;;
Request header: http://dev-odm2admin.cuahsi.org/odm2lczo/odm2lczo/soap/cuahsi_1_1/.wsdl
Request body:

<?xml version="1.0" encoding="utf-8"?><soap:Envelope 
xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema"><soap:Body><GetSiteInfo xmlns="http://www.cuahsi.org/his/1.1/ws/"><site>odm2lczo:Rio Icacos Trib-IO</site></GetSiteInfo></soap:Body></soap:Envelope>

we also had some trouble with routing due to the dot in the url (.wsdl)
do you need the dot in the route?

My response:

So if I change the envelope to:


<?xml version="1.0" encoding="utf-8"?>
<soap:Envelope 
xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" 
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<soap:Body>
<GetSiteInfo xmlns="http://www.cuahsi.org/his/1.1/ws/">
<site>Rio Icacos Trib-IO</site></GetSiteInfo>
</soap:Body></soap:Envelope>

It works

Where as, in the highlight section, you have:
<site>odm2lczo:Rio Icacos Trib-IO</site>
That would seem to be the service title. Can that be handled on your end?

@miguelcleon
Copy link
Member

@lsetiawan see my last comment. seems like this <site>odm2lczo:Rio Icacos Trib-IO</site> was working before not sure I named the service odm2lczo though.

@miguelcleon
Copy link
Member

From @martinseul

It’s convention to include the service title for the request all WOF services work that way.

So the odm2lczo: should be accetable

@emiliom
Copy link
Member Author

emiliom commented Feb 15, 2018

Thanks for those reports, @miguelcleon.

If you're available late Friday morning (Pacific Time), let's plan on touching base then. I'll be out of town Thursday and @lsetiawan will be working for another PI.

I feel I need to first review where we got late last year. I'm pretty sure the . in .wsdl had never been a problem, and we had gotten most of the request types to succeed on the CUAHSI HIS QA system. So, w/o actually having any evidence right now, I feel that there's a regression here, relative to last November or so. I'm confused.

FYI, I don't know that the . in .wsdl is necessary on our end, but it's been like that at least since we took on WOFpy, and possibly much longer.

@miguelcleon
Copy link
Member

Yes, OK, looks like my service name is actually woflczo so the below soap envelope works. I had changed the name, but I didn't change it on the HIS central QA system.

This one works http://qa-hiscentral.cuahsi.org/testpage.aspx?n=100005

Looks like I might need to have Martin delete the old one as I can't get it to update http://qa-hiscentral.cuahsi.org/testpage.aspx?n=100002

<?xml version="1.0" encoding="utf-8"?>
<soap:Envelope 
xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" 
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<soap:Body>
<GetSiteInfo xmlns="http://www.cuahsi.org/his/1.1/ws/">
<site>woflczo:Rio Icacos Trib-IO</site></GetSiteInfo>
</soap:Body></soap:Envelope>

@miguelcleon
Copy link
Member

So things appear to be working after all. I asked @martinseul to ingest the new service.

@emiliom
Copy link
Member Author

emiliom commented Feb 15, 2018

Great to hear, @miguelcleon! And also a big relief. Thanks for the updates.

@miguelcleon
Copy link
Member

miguelcleon commented Feb 15, 2018

from @martinseul

This looks good! It harvested fine and I can see and visualize on the client. Please check it out http://qa-hiswebclient.azurewebsites.net/#

Almost there!

If you search for Luquillo you can find two time series. There should be a third at another location but that isn't showing up and I've asked Martin about that. WOFpy is pointing to a very limited database for testing.

@emiliom
Copy link
Member Author

emiliom commented Feb 16, 2018

@miguelcleon It seems odd or suspicious that the Rio Icacos site only shows DO, and no temperature or conductivity. Is that what you've set up and what you expect? Just checking ...

@miguelcleon
Copy link
Member

@emiliom yeah, that is just what I put in this test database. Once the other site shows up on the test his web client I'll try a bigger database with more time series.

@miguelcleon
Copy link
Member

Rio Icacos is the site that is not showing up but it only has the one time series associated with it.

@emiliom
Copy link
Member Author

emiliom commented Feb 16, 2018

Ok, thanks.

@miguelcleon
Copy link
Member

miguelcleon commented Feb 23, 2018

From @martinseul

It seems you have spaces in the sitecode, could you remove them as this seems to create a problem for the harvesting, I’ll see on our end as well if we can mitigate this. In odm 1.1.1 allows sitecode in the range of AZ (case insensitive) , 0-9, and “.”, “-“, and “_”.
https://www.cuahsi.org/uploads/pages/img/ODM1.1DesignSpecifications_.pdf
Let me know and I try to reharvest.

Perhaps WOFpy should replace spaces in the sitecode with dashes or underscores. I'm not sure @emiliom ?

@miguelcleon
Copy link
Member

From @martinseul

Just harvested and the site does show up now and I can download as well!

Looks good, we will try a more complete database now.

@emiliom
Copy link
Member Author

emiliom commented Feb 23, 2018

Perhaps WOFpy should replace spaces in the sitecode with dashes or underscores.

Thanks for that follow up @miguelcleon. Changing WOFpy along those lines is certainly a possibility. But another parallel one is imposing constraints on ODM2 to begin with. I've opened a separate issue (#226) to discuss this.

Just harvested and the site does show up now and I can download as well!

How did you get past the sitecode bad-characters issue? Did you change the sitecode on your database, or did Martin develop mitigation steps on his end?

@miguelcleon
Copy link
Member

I just replaced the spaces with dashes.

@miguelcleon
Copy link
Member

miguelcleon commented Feb 26, 2018

OK, @martinseul ingested a nearly complete LCZO database, 12 out of 16 sites ingested properly but a few are erroring out. I'm not sure why. The problem would seem to be on the WOFpy side as I'm getting errors via REST. I spent some time trying to debug the problem but I haven't been able to figure it out

this site works and was succesfully ingested: https://dev-odm2admin.cuahsi.org/odm2lczo/odm2lczo/rest/1_1/GetSiteInfo?site=woflczo:RESSH

works: http://dev-odm2admin.cuahsi.org/odm2lczo/odm2lczo/rest/1_1/GetSites?site=woflczo:Prieta

doesn't work: http://dev-odm2admin.cuahsi.org/odm2lczo/odm2lczo/rest/1_1/GetSiteInfo?site=woflczo:Prieta

I can get @lsetiawan a copy of the database, I doubt this problem can be diagnosed without it.

Server side error:


[Mon Feb 26 20:14:33.705298 2018] [wsgi:error] [pid 121966:tid 140214398777088] ERROR:spyne.application:Fault(Server: '17252L')
[Mon Feb 26 20:14:33.705371 2018] [wsgi:error] [pid 121966:tid 140214398777088] Traceback (most recent call last):
[Mon Feb 26 20:14:33.705429 2018] [wsgi:error] [pid 121966:tid 140214398777088]   File "/home/azureadmin/miniconda2/envs/wofpy6/lib/python2.7/site-packages/spyne/application.py", line 151, in process_request
[Mon Feb 26 20:14:33.705442 2018] [wsgi:error] [pid 121966:tid 140214398777088]     ctx.out_object = self.call_wrapper(ctx)
[Mon Feb 26 20:14:33.705448 2018] [wsgi:error] [pid 121966:tid 140214398777088]   File "/home/azureadmin/miniconda2/envs/wofpy6/lib/python2.7/site-packages/spyne/application.py", line 235, in call_wrapper
[Mon Feb 26 20:14:33.705453 2018] [wsgi:error] [pid 121966:tid 140214398777088]     retval = ctx.descriptor.service_class.call_wrapper(ctx)
[Mon Feb 26 20:14:33.705458 2018] [wsgi:error] [pid 121966:tid 140214398777088]   File "/home/azureadmin/miniconda2/envs/wofpy6/lib/python2.7/site-packages/spyne/service.py", line 209, in call_wrapper32u
[Mon Feb 26 20:14:33.705464 2018] [wsgi:error] [pid 121966:tid 140214398777088]     return ctx.function(ctx, *args)
[Mon Feb 26 20:14:33.705469 2018] [wsgi:error] [pid 121966:tid 140214398777088]   File "/home/azureadmin/miniconda2/envs/wofpy6/lib/python2.7/site-packages/wof/apps/spyned_1_1.py", line 107, in GetSiteInfo
[Mon Feb 26 20:14:33.705482 2018] [wsgi:error] [pid 121966:tid 140214398777088]     siteinfoResult = WOFService.GetSiteInfoObject(ctx, site, authToken)
[Mon Feb 26 20:14:33.705487 2018] [wsgi:error] [pid 121966:tid 140214398777088]   File "/home/azureadmin/miniconda2/envs/wofpy6/lib/python2.7/site-packages/wof/apps/spyned_1_1.py", line 97, in GetSiteInfoObject
[Mon Feb 26 20:14:33.705492 2018] [wsgi:error] [pid 121966:tid 140214398777088]     raise Fault(faultstring=str(inst))
[Mon Feb 26 20:14:33.705497 2018] [wsgi:error] [pid 121966:tid 140214398777088] Fault: Fault(Server: '17252L')

@lsetiawan
Copy link
Member

I can get @lsetiawan a copy of the database, I doubt this problem can be diagnosed without it.

That'd be great. Thanks.

@emiliom
Copy link
Member Author

emiliom commented Feb 26, 2018

That's great news, @miguelcleon!

Regarding failing sites: we know your database has idiosyncracies. Some of those may be tough to identify, and probably even tougher to address -- on your end, most likely, or via a DAO customized to your database. SO:

  1. I suggest you go live with the LCZO WOFpy with the sites that work (12 out of 16 is pretty darn good!). We can't know how long it'll take us to address the rest, and we can't say how much time Don and I will have to address these subtleties.
  2. Let's close this monster, awful, rambling github issue and start one or more new ones that address specific problems!

@miguelcleon
Copy link
Member

@emiliom OK sounds good. Yes, we have made a lot of progress. Thanks much!

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

No branches or pull requests

4 participants