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

Return an AttrDict from get() instead of a bytes-encoded json string #86

Open
jesper-friis opened this issue Nov 19, 2022 · 3 comments · May be fixed by #134
Open

Return an AttrDict from get() instead of a bytes-encoded json string #86

jesper-friis opened this issue Nov 19, 2022 · 3 comments · May be fixed by #134
Assignees

Comments

@jesper-friis
Copy link
Contributor

Since the purpose of OTELib is to provide an user-friendly interface to oteapi-services, the get() method of a pipeline should return an AttrDict instead of a bytes-encoded json string.

@CasperWA
Copy link
Contributor

I think this is a nice idea. Even better would be to return a DLite instance if possible, when using DLite, but that's beyond this issue, I think.
Also, it shouldn't matter if it's an AttrDict or a regular Python dict. AttrDict is just a wrapper implementation for wrapping a standard dict in a pydantic BaseModel.

@daniel-sintef
Copy link
Contributor

I really value backwards-compatibility and don't like changing the system if we can avoid it. What might make more sense would be to provide an optional keyword argument to the .get() method that then allow the user to make the return output an AttrDict. Indeed this could be a very nice option as we could even add support for other datastructures down the line

@jesper-friis
Copy link
Contributor Author

In that case I will suggest to add a new execute() method with the new behaviour and deprecate get(). I anyway thing that get() is a misleading name of this method, see #108.

Note that the intention of this PR is not to ensorange people to put a lot of stuff in the session, but have a standardised way to e.g. getting hold of the collection_id. For this to be really useful, it should be possible to configure oteapi-services/otelib to use a given storage backend for all instances.

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.

3 participants