planetarypy / pvl

Python implementation of PVL (Parameter Value Language)
BSD 3-Clause "New" or "Revised" License
19 stars 19 forks source link

[WIP] adding http/https support for pvl load #50

Closed AndrewAnnex closed 4 years ago

AndrewAnnex commented 4 years ago

adds url support to load using urllib to allow remote file support, currently very simple, but this should be expanded to work against comparable list of sources as the GDAL virtual file system drivers. I think using a library (Apache Libcloud or other) as an optional dependency could help this work.

as a bonus I tried to make a unit test against https://pds-rings.seti.org/holdings/calibrated/COISS_2xxx_v1.0/COISS_2070/data/1695761485_1695858003/N1695761485_1_CALIB.LBL but ran into a parsing issue

AndrewAnnex commented 4 years ago

I suppose it is worth having a discussion if this is worth having in pvl as well.

my justification is that pvl files can be anywhere (local fs, cloud etc) and are typically co-located with their .IMG files and what have you. Given that gdal can read pds img files using vsicurl/vsis3/etc via gdal, it makes some sense to allow the equivalent path for the label to be used with pvl, and so that a higher level library that reads pds data can do less heavy lifting.

alternatively, of course it is easy enough for users to write their own requests/libcloud code themselves. so this could be considered out of scope for pvl

rbeyer commented 4 years ago

In the future, when this happens, log an Issue. I just did for this one via #51.

as a bonus I tried to make a unit test against https://pds-rings.seti.org/holdings/calibrated/COISS_2xxx_v1.0/COISS_2070/data/1695761485_1695858003/N1695761485_1_CALIB.LBL but ran into a parsing issue

rbeyer commented 4 years ago

On the one hand, my first reaction is: no, we parse strings and on-disk files, full stop. If a user needs to parse PVL-text from some other source they should handle getting that text as a string, and then use pvl.loads().

On the other hand, you've demonstrated that this doesn't require a lot of code, and has the potential to be really handy.

I've got some questions that I'll pose in individual line-based comments, but one of the bigger questions I had is that I see you're trying to modify get_text_from(path) to also handle URLs. That function was really meant to handle files, and I'm not sure that the logic is really extensible to handling the URL request process. Maybe we need to create a new pvl.loadu() that takes URLs? Did you think about that and dismiss it? Maybe it can all be done in get_text_from().

AndrewAnnex commented 4 years ago

Did you think about that and dismiss it? Maybe it can all be done in get_text_from()

nope I didn't think about it at all really, I just wanted to make a quick POC. probably what makes more sense is for the delegation on the url to occur in the load function instead of get_text_from and or within a load u function

AndrewAnnex commented 4 years ago

closing for #53