Feature for more io message pack targets #1242
Conversation
Signed-off-by: Steffan Wullems <129064935+SteffanWullems@users.noreply.github.com>
src/power_grid_model/utils.py
Outdated
| import warnings | ||
| from pathlib import Path | ||
| from typing import cast as cast_type | ||
| from typing import IO as mp_IO, Any as mp_Any, cast as cast_type |
There was a problem hiding this comment.
what does the prefix mp mean? Since you're naming it ByteIO later on, why not alias as ByteIO here? or just keep it as IO
There was a problem hiding this comment.
reason is IO and Any can clash
There was a problem hiding this comment.
Also IO[Any] allows all IO streams
There was a problem hiding this comment.
but what does mp_ mean?
if name clashes are the only reason, please instead use module-private prefixes
| from typing import IO as mp_IO, Any as mp_Any, cast as cast_type | |
| from typing import IO as _IO, Any as _Any, cast as cast_type |
tests/unit/test_serialization.py
Outdated
| def test_messagepack_round_trip_to_io(serialized_data): | ||
| data = to_msgpack(serialized_data) | ||
| input_data: Dataset = msgpack_deserialize(data) | ||
|
|
||
| io_buffer_data = BytesIO() |
There was a problem hiding this comment.
since the input type is IO[Any], we should test both BytesIO and TextIO.
EDIT: I implemented this and it fails (exception is raised during deserialization)
There was a problem hiding this comment.
The issue is if you set BytesIO it will not work for all bytes based streams.
There was a problem hiding this comment.
i'd still prefer that over things that we say we support but in reality don't
|
@SteffanWullems Can you please sign your commits? I'll make a full review first thing on Monday morning. |
figueroa1395
left a comment
figueroa1395
left a comment
There was a problem hiding this comment.
Looks good to me, just some minor comments.
| @@ -1,3 +1,4 @@ | |||
| # SPDX-FileCopyrightText: 2025 Contributors to the Power Grid Model project <powergridmodel@lfenergy.org> | |||
There was a problem hiding this comment.
Can you remove this line? Where did it come from?
|
|
||
|
|
||
| def msgpack_deserialize_from_stream( | ||
| stream: IO[bytes], |
There was a problem hiding this comment.
Nitpick: Here and elsewhere
| stream: IO[bytes], | |
| stream: BinaryIO, |
| if stream is IO[Any]: | ||
| raise TypeError("Expected a stream.") |
There was a problem hiding this comment.
I believe this is broken and does nothing. typing.IO is to be used only for type hinting and this check should always be false. I think it can be removed.
| if stream is IO[Any]: | ||
| raise TypeError("Expected a stream.") |
Signed-off-by: Steffan <steffan.wullems@alliander.com>
Signed-off-by: Steffan <steffan.wullems@alliander.com>
Signed-off-by: Steffan <steffan.wullems@alliander.com>
Signed-off-by: Steffan <steffan.wullems@alliander.com>
SteffanWullems
force-pushed
the
feature-for-more-IO-message-pack-targets
branch
from
e4ce485 to
58b6896
Compare
|
LGTM after resolution of comments |
4 participants
Changes proposed in this PR include:
This update introduces support for additional targets with read/write I/O capabilities,
including local storage, remote file systems, I/O buffers, and similar resources.
For example (using https://pypi.org/project/smart-open/):
`from smart_open import open as open_s3_file
TARGET_FILENAME="results.mgp"
S3_PATH = f"s3://some-aws-bucket/{TARGET_FILENAME}"
with open_s3_file( S3_PATH, "wb", transport_params={"client": s3client}) as fp:
msgpack_serialize_to_fileobj(fp, results)
with open_s3_file( S3_PATH, "rb", transport_params={"client": s3client}) as fp:
loaded_s3 = msgpack_deserialize_from_fileobj(fp)`