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

Allow keyword argument in get(asobj=True) #191

Open
huard opened this issue Mar 15, 2021 · 7 comments · May be fixed by #213
Open

Allow keyword argument in get(asobj=True) #191

huard opened this issue Mar 15, 2021 · 7 comments · May be fixed by #213
Assignees

Comments

@huard
Copy link
Contributor

huard commented Mar 15, 2021

Description

Some converters could accept keyword arguments. For example, some xarray datasets should be opened with decode_timedelta=False, which at the moment is not possible using get.

Environment

  • Birdy version used, if any: 0.7.0
@huard
Copy link
Contributor Author

huard commented Mar 31, 2022

I suggest we discuss how to do this cleanly.

The first option is simply to call get(asobj=True, **kwds), and then in the code I transfer those kwds to the convert method. My concern with this is that it's obscure; as a user you don't really know what function is going to be used to open the file, and you don't have access to the signature or docstring. If it fails, good luck.

Another option is to deprecate asobj=True, and attach the convert method to the output. So you would do something like:

out = response.get()
out.open(**kwds)

and would be able to inspect help(out.open). I think we could use boltons.funcutils.wraps to do this. A major difference is that out would be an Output instance, not just a string.

We could also leave get as is, and create another method, so we don't break anything.

Thoughts, other ideas ?

@huard
Copy link
Contributor Author

huard commented Mar 31, 2022

@cehbrecht Are you using this asobj functionality in rooki ? Would this break anything on your end ?

@huard
Copy link
Contributor Author

huard commented Mar 31, 2022

I've got something that would let you do:

>>> wps = WPSClient("http://localhost:5000")
>>> resp = wps.ncml()
>>> [d1, d2, ncml] = resp.load()  # Returns augmented owslib.wps.Output object that have a load method. 
>>> ds = d1.load(decode_times=False)
>>> help(d1.load)
Help on function open_dataset in module xarray.backends.api:

open_dataset(filename_or_obj, group=None, decode_cf=True, mask_and_scale=None, decode_times=True, autoclose=None, concat_characters=True, decode_coords=True, engine=None, chunks=None, lock=None, cache=None, drop_variables=None, backend_kwargs=None, use_cftime=None, decode_timedelta=None)
    Open and decode a dataset from a file or file-like object.
    
    Parameters
    ----------
    filename_or_obj : str, Path, file-like or DataStore
        Strings and Path objects are interpreted as a pat

One issue is that there is no nesting. If you load a metalink or zip, we cannot attach a load method to their content, at least not without doing some horrible coding.

@cehbrecht
Copy link
Member

@cehbrecht Are you using this asobj functionality in rooki ? Would this break anything on your end ?

rooki is just using the get to retrieve the metalink download URL:
https://github.com/roocs/rooki/blob/b093bc544280d87ebaf74ddcf8cac6535e33a514/rooki/results.py#L33

@cehbrecht
Copy link
Member

Another option is to deprecate asobj=True, and attach the convert method to the output. So you would do something like:

out = response.get()
out.open(**kwds)

I would also be in favor of using adapted methods for outputs.

To be on the save side we could deprecate the existing get function to avoid breaking our existing notebooks.

@cehbrecht
Copy link
Member

One issue is that there is no nesting. If you load a metalink or zip, we cannot attach a load method to their content, at least not without doing some horrible coding.

the load method for a metalink output could maybe implement some of the things we currently have in rooki:
https://github.com/roocs/rooki/blob/master/rooki/results.py

@cehbrecht
Copy link
Member

cehbrecht commented Apr 1, 2022

@huard If you choose a new method we need to decide for a new method name, excluding get: load, open, retrieve, result, output, ..., ?

The load method could return a new Result object which can then have access interpretations for the different output types (literal, mime-types).

@huard huard self-assigned this Apr 4, 2022
@huard huard linked a pull request Apr 4, 2022 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants