Skip to content

Raise the intended error when a space type has no deserializer - #345

Open
DenisDrobyshev wants to merge 2 commits into
Farama-Foundation:mainfrom
DenisDrobyshev:deserialize-unsupported-space-error
Open

DenisDrobyshev wants to merge 2 commits into
Farama-Foundation:mainfrom
DenisDrobyshev:deserialize-unsupported-space-error

Conversation

@DenisDrobyshev

Copy link
Copy Markdown

Description

deserialize_space reports an unregistered space type with the wrong error:

>>> from minari.serialization import deserialize_space
>>> deserialize_space({"type": "Graph"})
TypeError: deserialize_space() missing 1 required positional argument: 'space_dict'

type_value_dispatch builds its table as defaultdict(func). defaultdict
calls its factory with no arguments, so func — the fallback, which takes
space_dict — was invoked without it. The NotImplementedError it is written
to raise never got the chance:

@type_value_dispatch
def deserialize_space(space_dict: Dict) -> spaces.Space:
    raise NotImplementedError(
        f"No deserialization method available for {space_dict['type']}"
    )

The factory now returns the fallback rather than being it, and the message
names the type that has no deserializer.

The two directions disagreed until now: serialize_space is a singledispatch
and reports unsupported spaces correctly, which is what
test_space_serialize_deserialize_unsupported covers. Nothing exercised the
deserializing direction, so this went unnoticed.

Tests

tests/test_serialization.py gains test_space_deserialize_unsupported,
parametrised over Graph, Sequence and OneOf — three Gymnasium spaces with
no deserializer registered. All three fail on main with the TypeError above
and pass here; the file goes from 19 to 22 passing.

pre-commit run is clean on both files. tests/test_namespace.py fails 9 cases
here both with and without this change — they reach the remote storage, and the
baseline is the same.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with pre-commit run --all-files
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective

type_value_dispatch built its table as defaultdict(func). defaultdict calls
its factory with no arguments, so an unregistered type called the fallback
without space_dict and the caller saw

    TypeError: deserialize_space() missing 1 required positional argument

instead of the NotImplementedError the fallback is written to raise. The
factory now returns the fallback rather than being it.

serialize_space already reported unsupported types correctly, and only that
direction was covered by a test.
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 this pull request may close these issues.

2 participants