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

DevOps: Add AiiDA deprecation warnings #45

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 10, 2022

The warnings are enabled by setting the AIIDA_WARN_v3 environment variable to True in the CI workflow.

Most of the warnings are addressed, but a few remain that cannot be trivially fixed:

  • The id property on the aiida.orm.Entity class is deprecated. This is being called because the ORM proxy model declares id as the primary key field. This is correct for the database models of the PsqlDosBackend in aiida-core, but the front-end ORM is using the pk property instead.
  • The attributes, extras and repository_metadata of the proxy models are declared "top-level" and so pydantic will set them on the ORM instance using that property, however, these properties have been deprecated in aiida-core and should be set through the base.attributes.set_many, base.extras.set_many and the base.repository.metadata properties instead.

With the current setup using pydantic it is not clear how these changes can be addressed and whether the manner in which field values are being set on the underlying ORM model instance can be customized.

The warnings are enabled by setting the `AIIDA_WARN_v3` environment
variable to `True` in the CI workflow.

Most of the warnings are addressed, but a few remain that cannot be
trivially fixed:

 * The `id` property on the `aiida.orm.Entity` class is deprecated. This
   is being called because the ORM proxy model declares `id` as the
   primary key field. This is correct for the database models of the
   `PsqlDosBackend` in `aiida-core`, but the front-end ORM is using the
   `pk` property instead.
 * The `attributes`, `extras` and `repository_metadata` of the proxy
   models are declared "top-level" and so `pydantic` will set them on
   the ORM instance using that property, however, these properties have
   been deprecated in `aiida-core` and should be set through the
   `base.attributes.set_many`, `base.extras.set_many` and the
   `base.repository.metadata` properties instead.

With the current setup using `pydantic` it is not clear how these
changes can be addressed and whether the manner in which field values
are being set on the underlying ORM model instance can be customized.
@ltalirz
Copy link
Member

ltalirz commented Nov 10, 2022

  • The id property on the aiida.orm.Entity class is deprecated. This is being called because the ORM proxy model declares id as the primary key field. This is correct for the database models of the PsqlDosBackend in aiida-core, but the front-end ORM is using the pk property instead.

Hehe, so the PK made its way from being the primary key in the database backend to the frontend, and in the meanwhile it's actually gone from the backend?
Should we change the backend column name from id to pk then?

@sphuber
Copy link
Contributor Author

sphuber commented Nov 10, 2022

Hehe, so the PK made its way from being the primary key in the database backend to the frontend, and in the meanwhile it's actually gone from the backend?
Should we change the backend column name from id to pk then?

From the very beginning the database models chose id as the primary key, and Django provides the pk as alias. In the front end, we encouraged using pk, since id is a reserved keyword in Python. That's why we deprecated the id property on the front end ORM, however, the since the QueryBuilder still requires projecting and filtering on id since that operates (currently) directly on the database models, we never removed it.

@chrisjsewell actually opened a PR on aiida-core quite some time ago, that would correct this discrepancy in user interface. We should definitely look into this again and include it in the discussion of providing a single client-facing schema for both Python API, as well as other API's such as the web API.

@sphuber sphuber merged commit be87fe7 into master Nov 11, 2022
@sphuber sphuber deleted the fix/aiida-deprecation-warnings branch November 11, 2022 08:24
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