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.

Sign up for GitHub

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

Jump to bottom

Adding options to the Python plugin from_bytes() and to_bytes() methods #880

Merged
merged 8 commits into from
Jul 6, 2024
Merged

Adding options to the Python plugin from_bytes() and to_bytes() methods #880

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
325b1b2
Fixed adding options to the Python plygin from_bytes() and to_bytes()…
jesper-friis Jul 5, 2024
4d66bcb
Added options to docstring in from_bytes() and to_bytes() in the
jesper-friis Jul 5, 2024
15f9452
Merge branch 'master' into 868-add-options-to-instanceto_bytes-and-in…
jesper-friis Jul 5, 2024
3647ff0
Update bindings/python/dlite-python.i
jesper-friis Jul 5, 2024
f87072e
Update storages/python/dlite-plugins-python.c
jesper-friis Jul 5, 2024
8712eaf
Update storages/python/dlite-plugins-python.c
jesper-friis Jul 5, 2024
c8496db
Corrected typo
jesper-friis Jul 5, 2024
ef5b08b
Merge branch '868-add-options-to-instanceto_bytes-and-instancefrom_by…
jesper-friis Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bindings/python/dlite-entity-python.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,11 +486,11 @@ def get_instance(
)

@classmethod
def from_bytes(cls, driver, buffer, id=None):
def from_bytes(cls, driver, buffer, id=None, options=None):
"""Load the instance with ID `id` from bytes `buffer` using the
given storage driver.
"""
return _from_bytes(driver, buffer, id=id)
return _from_bytes(driver, buffer, id=id, options=options)

@classmethod
def create_metadata(cls, uri, dimensions, properties, description):
Expand Down
16 changes: 10 additions & 6 deletions bindings/python/dlite-entity.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,9 +421,10 @@ Call signatures:
%feature("docstring",
"Save instance to bytes using given storage driver.") to_bytes;
%newobject to_bytes;
void to_bytes(const char *driver, unsigned char **ARGOUT_BYTES, size_t *LEN) {
void to_bytes(const char *driver, unsigned char **ARGOUT_BYTES, size_t *LEN,
const char *options=NULL) {
unsigned char *buf=NULL;
int m, n = dlite_instance_memsave(driver, buf, 0, $self);
int m, n = dlite_instance_memsave(driver, buf, 0, $self, options);
if (n < 0) {
*ARGOUT_BYTES = NULL;
*LEN = 0;
Expand All @@ -433,7 +434,8 @@ Call signatures:
dlite_err(dliteMemoryError, "allocation failure");
return;
}
if ((m = dlite_instance_memsave(driver, buf, n, $self)) < 0) return;
if ((m = dlite_instance_memsave(driver, buf, n, $self, options)) < 0)
return;
assert (m == n);
*ARGOUT_BYTES = buf;
*LEN = n;
Expand Down Expand Up @@ -836,9 +838,11 @@ bool dlite_instance_has_property(struct _DLiteInstance *inst, const char *name);

%rename(_from_bytes) dlite_instance_memload;
%feature("docstring", "Loads instance from string.") dlite_instance_memload;
struct _DLiteInstance *dlite_instance_memload(const char *driver,
unsigned char *INPUT_BYTES, size_t LEN,
const char *id=NULL);
struct _DLiteInstance *
dlite_instance_memload(const char *driver,
unsigned char *INPUT_BYTES, size_t LEN,
const char *id=NULL, const char *options=NULL);


/* FIXME - how do we avoid duplicating these constants from dlite-schemas.h? */
#define BASIC_METADATA_SCHEMA "http://onto-ns.com/meta/0.1/BasicMetadataSchema"
Expand Down
2 changes: 1 addition & 1 deletion bindings/python/dlite-python.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,7 +1289,7 @@ int dlite_swig_set_property_by_index(DLiteInstance *inst, int i, obj_t *obj)
* --------------- */

/* Argout bytes */
/* We assumes that *ARGOUT_BYTES is a malloc() by the wrapped function */
/* We assumes that *ARGOUT_BYTES is malloc()'ed by the wrapped function */
jesper-friis marked this conversation as resolved.
Show resolved Hide resolved
%typemap("doc") (unsigned char **ARGOUT_BYTES, size_t *LEN) "bytes"
%typemap(in,numinputs=0) (unsigned char **ARGOUT_BYTES, size_t *LEN)
(unsigned char *tmp, size_t n) {
Expand Down
11 changes: 10 additions & 1 deletion bindings/python/tests/test_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,20 @@

# Test to_bytes()/from_bytes()
data = inst.to_bytes("yaml")
data2 = data.replace(b"uri: my-data", b"uri: my-data2")
data2 = data.replace(b"uri: my-data", b"uri: my-data2").replace(
dlite.get_uuid("my-data").encode(),
dlite.get_uuid("my-data2").encode()
)
assert data2.startswith(b"uri: my-data2")

inst2 = dlite.Instance.from_bytes("yaml", data2)
assert inst2.uuid != inst.uuid
assert inst2.get_hash() == inst.get_hash()

# Test to_bytes() with options
data3 = inst.to_bytes("yaml", options="single=false")
assert data3.startswith(b"ce592ff9-a7dc-548a-bbe5-3c53800afaf3")

s.flush() # avoid calling flush() when the interpreter is teared down


Expand Down
6 changes: 4 additions & 2 deletions doc/user_guide/storage_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,24 +67,26 @@ def queue(self, pattern=None):
"""

@classmethod
def from_bytes(cls, buffer, id=None):
def from_bytes(cls, buffer, id=None, options=None):
"""Load instance with given `id` from `buffer`.

Arguments:
buffer: Bytes or bytearray object to load the instance from.
id: ID of instance to load. May be omitted if `buffer` only
holds one instance.
options: Options string for this storage driver.

Returns:
New instance.
"""

@classmethod
def to_bytes(cls, inst):
def to_bytes(cls, inst, options=None):
"""Save instance `inst` to bytes (or bytearray) object. Optional.

Arguments:
inst: Instance to save.
options: Options string for this storage driver.

Returns:
The bytes (or bytearray) object that the instance is saved to.
Expand Down
15 changes: 8 additions & 7 deletions src/dlite-entity.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1144,14 +1144,14 @@ int dlite_instance_save_url(const char *url, const DLiteInstance *inst)
*/
DLiteInstance *dlite_instance_memload(const char *driver,
const unsigned char *buf, size_t size,
const char *id)
const char *id, const char *options)
{
const DLiteStoragePlugin *api;
if (!(api = dlite_storage_plugin_get(driver))) return NULL;
if (!api->memLoadInstance)
return err(dliteUnsupportedError, "driver does not support memload: %s",
api->name), NULL;
return api->memLoadInstance(api, buf, size, id);
return api->memLoadInstance(api, buf, size, id, options);
}

/*
Expand All @@ -1162,22 +1162,23 @@ DLiteInstance *dlite_instance_memload(const char *driver,
Returns a negative error code on error.
*/
int dlite_instance_memsave(const char *driver, unsigned char *buf, size_t size,
const DLiteInstance *inst)
const DLiteInstance *inst, const char *options)
{
const DLiteStoragePlugin *api;
if (!(api = dlite_storage_plugin_get(driver))) return dliteStorageSaveError;
if (!api->memSaveInstance)
return err(dliteUnsupportedError, "driver does not support memsave: %s",
api->name);
return api->memSaveInstance(api, buf, size, inst);
return api->memSaveInstance(api, buf, size, inst, options);
}

/*
Saves instance to a newly allocated memory buffer.
Returns NULL on error.
*/
unsigned char *dlite_instance_to_memory(const char *driver,
const DLiteInstance *inst)
const DLiteInstance *inst,
const char *options)
{
const DLiteStoragePlugin *api;
int n=0, m;
Expand All @@ -1187,10 +1188,10 @@ unsigned char *dlite_instance_to_memory(const char *driver,
return err(dliteUnsupportedError, "driver does not support memsave: %s",
api->name), NULL;

if ((n = api->memSaveInstance(api, buf, n, inst)) < 0) return NULL;
if ((n = api->memSaveInstance(api, buf, n, inst, options)) < 0) return NULL;
if (!(buf = malloc(n)))
return err(dliteMemoryError, "allocation failure"), NULL;
if ((m = api->memSaveInstance(api, buf, n, inst)) != n) {
if ((m = api->memSaveInstance(api, buf, n, inst, options)) != n) {
assert(m < 0); // On success, should `m` always be equal to `n`!
free(buf);
return NULL;
Expand Down
7 changes: 4 additions & 3 deletions src/dlite-entity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ int dlite_instance_save_url(const char *url, const DLiteInstance *inst);
*/
DLiteInstance *dlite_instance_memload(const char *driver,
const unsigned char *buf, size_t size,
const char *id);
const char *id, const char *options);

/**
Stores instance `inst` to memory buffer `buf` of size `size`.
Expand All @@ -638,14 +638,15 @@ DLiteInstance *dlite_instance_memload(const char *driver,
Returns a negative error code on error.
*/
int dlite_instance_memsave(const char *driver, unsigned char *buf, size_t size,
const DLiteInstance *inst);
const DLiteInstance *inst, const char *options);

/**
Saves instance to a newly allocated memory buffer.
Returns NULL on error.
*/
unsigned char *dlite_instance_to_memory(const char *driver,
const DLiteInstance *inst);
const DLiteInstance *inst,
const char *options);

/**
Returns a pointer to instance UUID.
Expand Down
5 changes: 3 additions & 2 deletions src/dlite-storage-plugins.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ typedef int (*DeleteInstance)(DLiteStorage *s, const char *id);
*/
typedef DLiteInstance *(*MemLoadInstance)(const DLiteStoragePlugin *api,
const unsigned char *buf, size_t size,
const char *id);
const char *id, const char *options);

/**
Stores instance `inst` to memory buffer `buf` of size `size`.
Expand All @@ -390,7 +390,8 @@ typedef DLiteInstance *(*MemLoadInstance)(const DLiteStoragePlugin *api,
*/
typedef int (*MemSaveInstance)(const DLiteStoragePlugin *api,
unsigned char *buf, size_t size,
const DLiteInstance *inst);
const DLiteInstance *inst,
const char *options);

/** @} */

Expand Down
26 changes: 21 additions & 5 deletions storages/python/dlite-plugins-python.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,8 @@ int deleter(DLiteStorage *s, const char *id)
Loads instance with given id from bytes object.
*/
DLiteInstance *memloader(const DLiteStoragePlugin *api,
const unsigned char *buf, size_t size, const char *id)
const unsigned char *buf, size_t size,
const char *options, const char *id)
{
DLiteInstance *inst = NULL;
PyObject *v = NULL;
Expand All @@ -337,8 +338,16 @@ DLiteInstance *memloader(const DLiteStoragePlugin *api,
PyErr_Clear();
if (!(classname = dlite_pyembed_classname(class)))
dlite_warnx("cannot get class name for storage plugin '%s'", api->name);
v = PyObject_CallMethod(class, "from_bytes", "y#s",
(const char *)buf, (Py_ssize_t) size, id);

/* Keep backward compatibility with Python plugins that doesn't have an
jesper-friis marked this conversation as resolved.
Show resolved Hide resolved
`options` argument in their `from_bytes()` method. */
if (options)
v = PyObject_CallMethod(class, "from_bytes", "y#ss",
(const char *)buf, (Py_ssize_t) size, id, options);
else
v = PyObject_CallMethod(class, "from_bytes", "y#s",
(const char *)buf, (Py_ssize_t) size, id);

if (dlite_pyembed_err_check("calling from_bytes() in Python plugin '%s'",
classname)) {
Py_XDECREF(v);
Expand All @@ -357,7 +366,7 @@ DLiteInstance *memloader(const DLiteStoragePlugin *api,
Saves instance to bytes object.
*/
int memsaver(const DLiteStoragePlugin *api, unsigned char *buf, size_t size,
const DLiteInstance *inst)
const DLiteInstance *inst, const char *options)
{
Py_ssize_t length = 0;
char *buffer = NULL;
Expand All @@ -370,7 +379,14 @@ int memsaver(const DLiteStoragePlugin *api, unsigned char *buf, size_t size,
if (!pyinst) goto fail;
if (!(classname = dlite_pyembed_classname(class)))
dlite_warnx("cannot get class name for storage plugin '%s'", api->name);
v = PyObject_CallMethod(class, "to_bytes", "O", pyinst);

/* Keep backward compatibility with Python plugins that doesn't have an
jesper-friis marked this conversation as resolved.
Show resolved Hide resolved
`options` argument in their `to_bytes()` method. */
if (options)
v = PyObject_CallMethod(class, "to_bytes", "Os", pyinst, options);
else
v = PyObject_CallMethod(class, "to_bytes", "O", pyinst);

if (dlite_pyembed_err_check("calling to_bytes() in Python plugin '%s'%s",
classname, failmsg()))
goto fail;
Expand Down
19 changes: 14 additions & 5 deletions storages/python/python-storage-plugins/yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def queue(self, pattern=None):
yield uuid

@classmethod
def from_bytes(cls, buffer, id=None):
def from_bytes(cls, buffer, id=None, options=None):
"""Load instance with given `id` from `buffer`.

Arguments:
Expand All @@ -148,19 +148,28 @@ def from_bytes(cls, buffer, id=None):
)

@classmethod
def to_bytes(cls, inst, soft7=True, with_uuid=False):
def to_bytes(cls, inst, options=None):
"""Save instance `inst` to bytes (or bytearray) object. Optional.

Arguments:
inst: Instance to save.
soft7: Whether to structure metadata as SOFT7.
with_uuid: Whether to include UUID in the output.
options: Supported options:
- `soft7`: Whether to structure metadata as SOFT7.
- `with_uuid`: Whether to include UUID in the output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have two argumenter for the same things?

Copy link
Collaborator Author

@jesper-friis jesper-friis Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

soft7 and with_uuid are not the same thing. With soft7=True dimensions and properties will be dicts, otherwise they will be arrays. with_uuid=True simply adds an uuid key to the return representation.
Or did I misunderstood your question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I measn with_uuid and single. They have the same description.

Copy link
Collaborator Author

@jesper-friis jesper-friis Jul 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For me the option descriptions look different:

    Arguments:
        location: Path to YAML file.
        options: Supported options:
        - `mode`: Mode for opening.  Valid values are:
            - `a`: Append to existing file or create new file (default).
            - `r`: Open existing file for read-only.
            - `w`: Truncate existing file or create new file.
        - `soft7`: Whether to save using SOFT7 format.
        - `single`: Whether the input is assumed to be in single-entity form.
          If "auto" (default) the form will be inferred automatically.
        - `with_uuid`: Whether to include UUID when saving.

The with_uuid=true option includes an "uuid" field next to "meta", "description", "dimensions" and "properties", while single=false creates another level of nesting using the UUID as key for the instance. So with single=true;with_uuid=false no UUID will be included in the output. While with single=false;with_uuid=true the UUID will be shown twice.

But I see some possible improvements:

  • with_uuid could be renamed to with-uuid for consistency with the json plugin
  • it would be more useful to use the single option for controlling for output (this is how it is used in the json plugin)
  • with PR Added support for to_bytes() and from_bytes() in the json storage plugin #881, the json plugin could be used directly, instead of relying on separate Python-implementations of Instance.asdict() and dlite.utils.instance_from_dict(). That would simplify things and improve consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just look at line 158 and 159 here (above and below this comment section. 'single' and 'with_uuid' have the same description.

I think it is very good to be consistent with the json plugin.

- `single`: Whether to include UUID in the output.

Returns:
The bytes (or bytearray) object that the instance is saved to.
"""
opts = Options(
options, defaults="soft7=true;with_uuid=false;single=true"
)
return pyyaml.safe_dump(
inst.asdict(soft7=soft7, uuid=with_uuid, single=True),
inst.asdict(
soft7=dlite.asbool(opts.soft7),
uuid=dlite.asbool(opts.with_uuid),
single=dlite.asbool(opts.single),
),
default_flow_style=False,
sort_keys=False,
).encode()