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

Migrate to pydantic v2 #504

Closed

Conversation

Leobouloc
Copy link
Contributor

Purpose

As mentionned in #401 , upgrading to pydantic 2 should greatly improve performances. Also it would allow projects depending on ralph (such as warren) to upgrade more easily.

Dealing with common errors when migrating

ERROR: Extra not permitted

In conf.py, errors such as:

    super().__init__(
E   pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
E   LOCALE_ENCODING
E     Extra inputs are not permitted [type=extra_forbidden, input_value='utf-8', input_type=str]
E       For further information visit https://errors.pydantic.dev/2.1/v/extra_forbidden

may be due to a change in default behavior in models extra=ignore to extra=failure. We may set this parameter manually to fix this issue.

ERROR: Unable to generate pydantic-core schema

E   pydantic.errors.PydanticSchemaGenerationError: Unable to generate pydantic-core schema for <class 'ralph.models.xapi.base.common.IRI'>. Set `arbitrary_types_allowed=True` in the model_config to ignore this error or implement `__get_pydantic_core_schema__` on your type to fully support it.
E
E   If you got this error by calling handler(<some type>) within `__get_pydantic_core_schema__` then you likely need to call `handler.generate_schema(<some type>)` since we do not call `__get_pydantic_core_schema__` on `<some type>` otherwise to avoid infinite recursion.
E

To fix this we change the way we define classes.

Previously in Ralph

class CommaSeparatedTuple(BaseModel):
    @model_validator(mode='before')
    @classmethod
    def validate(cls, value: Union[str, Tuple[str]]) -> Tuple[str]:
        """Checks whether the value is a comma separated string or a tuple."""

        if isinstance(value, tuple):
            return value

        if isinstance(value, str):
            return tuple(value.split(","))

        raise TypeError("Invalid comma separated list")

New in Ralph

def validate_comma_separated_tuple(value: Union[str, Tuple[str, ...]]) -> Tuple[str]:
    """Checks whether the value is a comma separated string or a tuple."""

    if isinstance(value, tuple):
        return value

    if isinstance(value, str):
        return tuple(value.split(","))

    raise TypeError("Invalid comma separated list")

CommaSeparatedTuple = Annotated[Union[str, Tuple[str, ...]], AfterValidator(validate_comma_separated_tuple)]

ERROR: Missing values

A recurrent error is the one below, which should be linked to the change of behavior when using Optional[...] in Pydantic V2. Previously, it would assign a default value of None, which is no longer the case. This should have been dealt with by bump-pydantic but was not always the case. It would be nice to know why before confirming this solution.

src/ralph/models/xapi/video/statements.py:85: in VideoPlayed
    verb: PlayedVerb = PlayedVerb()
E   pydantic_core._pydantic_core.ValidationError: 1 validation error for PlayedVerb
E   display
E     Field required [type=missing, input_value={}, input_type=dict]
E       For further information visit https://errors.pydantic.dev/2.1/v/missing

Previously in Ralph

class VideoActivity(BaseXapiActivity):
    name: Optional[Dict[Literal[LANG_EN_US_DISPLAY], str]]

New in Ralph

class VideoActivity(BaseXapiActivity):
    name: Optional[Dict[Literal[LANG_EN_US_DISPLAY], str]] = None

quitterie-lcs and others added 30 commits October 2, 2023 18:38
Some tests in edx to xapi converters were named without a pattern. The path
pattern used in other tests is applied for these tests.
`school`, `course` and `module` context extensions used in base edx-to-xapi
converter can no longer be used. The tracking of the course information
depends on the target statements templates used in xAPI profiles.
| datasource | package         | from   | to     |
| ---------- | --------------- | ------ | ------ |
| pypi       | hypothesis      | 6.87.1 | 6.87.3 |
| pypi       | mkdocs-material | 9.4.2  | 9.4.4  |
| pypi       | pylint          | 2.17.7 | 3.0.1  |
We intend to unify the storage and database backend interfaces
into a single data backend interface.
We also want to separate LRS-specific methods into a dedicated
lrs backend interface that extends the data interface.
We add the FileSystem data backend implementation that is mostly taken
from the existing FSStorage backend.
Updating List type to use List of Pydantic and handle Python3.8
Add Mongo backend under the new common 'data' interface.
With Mongo under the new data interface, tests are updated as well.
Storage and Database backends had similar interfaces and usage,
so a new Data Backend interface has been created.
We add the OpenStack Swift data backend implementation. With the `data`
parameter changed to an Iterable, we cannot use high level SwiftService API to
upload files anymore (it needs a file object source, not an iterable).
Changing to Connection lower-level API, which is more flexible.
We add the LDP data backend implementation that is mostly taken
from the existing LDPStorage backend.
Add S3 backend under the new common `data` interface
Add ClickHouse backend under the new common 'data' interface.
With ClickHouse under the new data interface, tests are updated as well.
Storage and Database backends had similar interfaces and usage,
so a new Data Backend interface has been created.
We want to simplify our tests that are mocking the request package.
Therefore we choose to use the `request_mock` library.
We add the ES data backend implementation that is mostly taken
from the existing ESDatabase backend.
We want to provide an LRS backend that could be added to
Ralph's LRS without any additional dependencies.
Methods `read_raw` and `parse_bytes_to_dict` are generic and used by multiple
backends. Moving them to file `utils.py`.
Add asynchronous base interface for async backends such as async_es or
async_mongo
We add the Async elasticsearch data backend mostly taken from the sync backend
using the async elasticsearch methods.
We want to improve the current mongo data backend implementation
to align it more with other unified data backends.
We want to provide an async version of our MongoDatabase
backend.
- `get_query` method for Elasticsearch would be better namespaced under the
ESLRSBackend.
Changing it to a static method instead of a global function.
- At initialization, data backends can either take settings or None.
Setting `settings_class` to Optional to anticipate mypy warning when mypy will
be added.
- Piping x|None is preferred since Python 3.10, changing from Optional to
Union[x|None] for backends as it would be easier to switch to pipes.
- Changes to backend methods docstrings
- Rename variable `new_documents` to be more explicit
Synchronous backends such as Elasticsearch or Mongo need their connection to
be closed when finished. This commits adds an abstract method close to the
BaseDataBackend interface, and implements it for backends that need it.
The update to a recent version of `motor` highlighted a bug on our side when
listing collections. Now asynchronously iterate over collections list.
With the new data backend interface, settings are now close to each backend
and not under general conf.py.
Unifying stream backend WS to have the same architecture as data backends.
With the new data backend interface, settings are now close to each backend
and not under general conf.py.
Unifying HTTP backends to have the same architecture as data backends.
After unifying database and storage backends under a common interface, backends
settings are now handled directly alongside the backends classes.
Modifying the CLI to support new settings and new backends interfaces.
With the addition of new asynchronous backends, it could be useful to be able
to use them in the CLI.
Adding a default value for ClickHouse client option
`allow_experimental_object_type` highlights a pydantic validation error with
type `Literal[0,1]`. Switching to `coint`.
With addition of unified backends and changes to the conf files, API router
needs some changes to be able to get the backends instance.
With addition of unified backends, API router needs some changes to be able to
use asynchronous backends.
Tests using filesystem failed with pyfakefs in the CI as pyfakefs does not
succeed on creating requesting files in the default directory path. The latter
is then defined specifically for these tests and forced to be used in the ralph
command.
renovate bot and others added 10 commits October 30, 2023 10:21
| datasource | package          | from     | to        |
| ---------- | ---------------- | -------- | --------- |
| pypi       | black            | 23.10.0  | 23.10.1   |
| pypi       | cachetools       | 5.3.1    | 5.3.2     |
| pypi       | cryptography     | 41.0.4   | 41.0.5    |
| pypi       | mkdocs-material  | 9.4.6    | 9.4.7     |
| pypi       | moto             | 4.2.6    | 4.2.7     |
| pypi       | mypy             | 1.2.0    | 1.6.1     |
| pypi       | pytest           | 7.4.2    | 7.4.3     |
| pypi       | types-cachetools | 5.3.0.6  | 5.3.0.7   |
| pypi       | types-requests   | 2.31.0.6 | 2.31.0.10 |
Changed:

- Upgrade `cachetools` to `5.3.2`
@@ -41,13 +42,13 @@ class CorrectMap(BaseModelWithConfig):
queuestate (json): see QueueStateField.
"""

answervariable: Union[Literal[None], None, str]
answervariable: Union[Literal[None], None, str] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"I wonder if setting a default value for this field makes it not required" Originally commented by @quitterie-lcs

@Leobouloc Leobouloc changed the base branch from master to feature/api/allow-multiple-auth-after-unification November 10, 2023 09:29
@Leobouloc Leobouloc force-pushed the feature/api/allow-multiple-auth-after-unification branch from 02ab20a to d62f720 Compare November 10, 2023 10:05
@wilbrdt wilbrdt added this to the 5.0 milestone Nov 14, 2023
@Leobouloc Leobouloc force-pushed the feature/api/allow-multiple-auth-after-unification branch 7 times, most recently from 1fe8b22 to e1889d2 Compare November 15, 2023 15:49
Base automatically changed from feature/api/allow-multiple-auth-after-unification to master November 15, 2023 15:57
@Leobouloc Leobouloc changed the base branch from master to feature/enforce-scopes December 19, 2023 09:30
@wilbrdt
Copy link
Contributor

wilbrdt commented Feb 12, 2024

Closing as it is done in #550

@wilbrdt wilbrdt closed this Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

7 participants