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

✨ NEW: Add orm.Entity.fields interface for QueryBuilder (cont.) #6245

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Jan 5, 2024

This PR (created following the suggestion of @sphuber) picks up the work of @chrisjsewell on PR #5088. The PR has been rebased onto main as of 11.03.2024.

The goal is to resolve any pending issues and concerns, then merge into main.

See full discussion at #5088.

Currently, all tests pass. Post-tests teardown errors are ignored for now (see comments on #5088 and #6134).

Update

Attempts to consolidate the logic of #6255 with this PR have hit a snag (see discussion below). Planning to merge this PR as is and address #6255 at a later time.

@edan-bainglass
Copy link
Member Author

@sphuber Opened! Had to fix a few ruff-related pre-commit fails (straightforward). But I'm running into a great deal of mypy errors (import-not-found, arg-type, etc.). Oddly, these are mostly occurring in places not touched by this PR. My question then is how did main get here? Surely I'm missing something.

@sphuber
Copy link
Contributor

sphuber commented Jan 5, 2024

@sphuber Opened! Had to fix a few ruff-related pre-commit fails (straightforward). But I'm running into a great deal of mypy errors (import-not-found, arg-type, etc.). Oddly, these are mostly occurring in places not touched by this PR. My question then is how did main get here? Surely I'm missing something.

Are you sure? From what I can see, most if not all are related to changes in this PR. Note that a warning can occur at a line that was not actually touched, but it uses a variable or calls a method that was. Since it checks for type consistency, if any type elsewhere was changed but the affected line wasn't, this can cause mypy to raise a warning.

As for why this didn't crop up in the original PR: the original PR was over 2 years old and back then we had an older version of mypy and had less strict type checking enabled. So this does not surprise me at all that we find new problems.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 5, 2024

Okay. No problem. I'll dig deeper and resolve these issues 👍


Dug just slightly deeper...

Cannot find implementation or library stub for module named "pytest"  [import-not-found]

How would your argument pertain to this error? Importing pytest surely has nothing to do with this PR. All pytest imports are failing w.r.t. mypy. Regarding your point that mypy is being used more strictly now, I ask the same question - how did main get here without raising these errors?


Answering my own question, at least for the the pytest part, I am running the pre-commits locally to sort out the errors. Since I did not have pytest installed, mypy couldn't find the module. A question - shouldn't the hook be running in its own virtual environment? If so, why the apparent dependency on a local module?

@edan-bainglass
Copy link
Member Author

One more thing...

The verdi-autodocs pre-commit hook removed the following from the docs:

.. _reference:command-line:verdi-tui:

``verdi tui``
-------------

.. code:: console

    Usage:  [OPTIONS]

      Open Textual TUI.

    Options:
      --help  Show this message and exit.

This was added by Chris in #6071.

What's happening here?

@sphuber
Copy link
Contributor

sphuber commented Jan 5, 2024

What's happening here?

This is because you don't have the optional dependency trogon. If you run pip install trogon it will keep the section.

@sphuber
Copy link
Contributor

sphuber commented Jan 5, 2024

A question - shouldn't the hook be running in its own virtual environment? If so, why the apparent dependency on a local module?

That is exactly why it is running as a local module, because it needs all the dependencies. When developing, you should install with at least pip install .[pre-commit,tests,atomic_tools,tui]. If we use the typical pre-commit approach of letting it run in its own virtual env, we would have to specify all the additional dependencies, which would amount to copy paste all dependencies we have in pyproject.toml in the .pre-commit-config.yaml. This would be a nightmare to keep synchronized.

@edan-bainglass
Copy link
Member Author

@sphuber I think perhaps my environment is not set up correctly? I checked out main (so no entity-fields PR) and reran the mypy pre-commit hook. Many errors!

@sphuber
Copy link
Contributor

sphuber commented Jan 8, 2024

@sphuber I think perhaps my environment is not set up correctly? I checked out main (so no entity-fields PR) and reran the mypy pre-commit hook. Many errors!

That indeed suggests your environment is not properly set up. I just ran pre-commit on your branch locally and I get exactly the same errors as the CI on Github actions.

What OS are you on, and how did you create your env? And how are you running pre-commit?

@edan-bainglass
Copy link
Member Author

What OS are you on, and how did you create your env? And how are you running pre-commit?

Windows -> WSL2 -> mamba python 3.9 environment

pip install .[pre-commit,tests,atomic_tools,tui]
pre-commit install
pre-commit run --all-files mypy

I was suspecting a collision of packages, so I uninstalled python, pip, and conda entirely, then reinstalled conda/mamba via miniforge. I recreated the environment and ran the above code block.

I get Found 783 errors in 115 files (checked 754 source files) as before.

Not sure what's going on. In the spirit of moving this along, gonna try running this in a container.

@sphuber
Copy link
Contributor

sphuber commented Jan 9, 2024

Hmm, that is weird. But for typing, a lot changed in Python 3.10 and that is what our CI is using. Could you try in an env with Python 3.10 or newer?

@edan-bainglass
Copy link
Member Author

Tested with 3.10 and 3.12. Same issue.

Consider the following:

src/aiida/restapi/resources.py:589: error: Incompatible types in assignment (expression has type "str", base class "BaseResource" defined the type as "None") [assignment]

In BaseResource:
_parse_pk_uuid = None

In Computer(BaseResource):
_parse_pk_uuid = 'uuid'

Do you expect mypy to ignore this?

@sphuber
Copy link
Contributor

sphuber commented Jan 9, 2024

I see what the problem is. I think the .pre-commit-config.yaml is getting ignored in your case. As you can see there that src/aiida/restapi/resources.py is ignored. I remember reading at some point that invoking pre-commit manually with --all-files does not take the same path as when it is invoked automatically through the commit hook. I cannot find the link now, but this seems to be the problem, which you can therefore ignore really.

@edan-bainglass
Copy link
Member Author

pre-commit/pre-commit#2962

Maybe 🤷🏻‍♂️

@sphuber
Copy link
Contributor

sphuber commented Jan 9, 2024

pre-commit/pre-commit#2962

Maybe 🤷🏻‍♂️

Either way, it is not relevant ultimately. The pre-commit on this PR is failing for genuine reasons with errors due to code introduced in the PR. Simply address those and then when you commit, your pre-commit should pass.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 9, 2024

pre-commit/pre-commit#2962
Maybe 🤷🏻‍♂️

Either way, it is not relevant ultimately. The pre-commit on this PR is failing for genuine reasons with errors due to code introduced in the PR. Simply address those and then when you commit, your pre-commit should pass.

Sorry 😅 The whole point of my original comment regarding mypy was that I was having difficulties differentiating between the failures due to this PR and those caused by an ill-configured environment and wanted to make sure I was not doing something dumb.

In the end, I was able to "calm" mypy down on an aiida-core-with-services Docker instance and can now clearly see those issues raised by this PR. Currently working through it.

I was referencing the pre-commit PR for future reference, in case anyone runs into similar issues. However, as I was having significantly less trouble taking the Docker route, @unkcpz and I were discussing if perhaps a dev version of the aiida-core-with-services (with aiida-core + optional deps pre-installed) might be a good option for new aiida-core developers. Will raise topic next meeting.

Thanks for the help @sphuber


See issue #5026 - running pre-commit via tox resolves the above issues locally.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 10, 2024

Okay, mypy issues resolved. See comments regarding a few uncertainties. Regardless, this is ready for final (🤞) review.

@edan-bainglass
Copy link
Member Author

One thing that is apparently missing is or functionality when using the comparator notation.

You can write

qb = QueryBuilder().append(
    Data,
    filters={
        "pk": 5,
        "label": "hello"
    }
)

as

qb = QueryBuilder().append(
    Data,
    filters={
        Data.fields.pk: 5,
        Data.fields.label: "hello"
    }
)

or, using comparators, as

qb = QueryBuilder().append(
    Data,
    filters=(Data.fields.pk == 5) & (Data.fields.label == "hello")
)

Also,

filters = (Data.fields.pk > 5) & (Data.fields.pk < 10)

is correctly translated into and 'and': [{'pk' > 5}, {'pk' < 10}].

However, I don't see a way you do or conditions. Specifically,

qb = QueryBuilder().append(
    Data,
    filters={
        "or": [
            {"pk": 5},
            {"label": "hello"}
        ]
    }
)

is not available through something like

(Data.fields.pk == 5) | (Data.fields.label == "hello")

Considering implementing it

@chrisjsewell
Copy link
Member

picks up the work of @chrisjsewell on PR #5088

Absolutely no problem, but maybe add me as a co-author in the final merge commit 😉: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

Co-authored-by: Chris Sewell <[email protected]>

@edan-bainglass
Copy link
Member Author

Absolutely no problem, but maybe add me as a co-author in the final merge commit 😉: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

Co-authored-by: Chris Sewell <[email protected]>

Duh 👍

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Jan 14, 2024

@sphuber, @chrisjsewell I tested the pydantic version of this PR. Some tests are failing with

assert TypeError("Parameter.__init__() got an unexpected keyword argument 'is_attribute'") is None

Anyhow, I rebases my work onto your branch and proceeded to work out model inheritance automation.

Consider the following:

class EntityChild(Entity, metaclass=EntityFieldMeta):
    class Model:
        attr: str = 'some_attr'

Note that Model does not explicitly inherit anything. This is ideal (not forcing anything on the user) but comes at a potential cost. My current implementation automates the model inheritance chain correctly in the sense that tests are passing (other than the ones already not passing on your branch) and the behavior is as expected. However, pydantic raises the following warning on each triggering of my code in EntityFieldMeta:

UserWarning: Field name "attr" shadows an attribute in parent "Entity.Model";

This is due to the particular means in which I am constructing the Model. Instead of adding bases to the Model nested class (which seems to be forbidden), I create a new Model inheriting the explicitly defined Model and those of the Entity parents. I believe this results in inheriting attr before attr is redefined in the body of EntityChild.Model. Nevertheless, it appears to not affect the results, but its quite suspect. The warnings can be avoided by explicitly inheriting BaseModel:

class EntityChild(Entity, metaclass=EntityFieldMeta):
    class Model(BaseModel):
        attr: str = 'some_attr'

but then we're back to requiring at least one base, and by extension, a check that the base is present. Not ideal 😕

Another concern is that the above leads to the following MRO:

Entity.Model.mro()

[abc.Model,
 aiida.orm.entities.Entity.Model,
 pydantic.main.BaseModel,
 object]

It gets worse down the inheritance chain, with abc.Model repeating for each level.

ProcessNode.Model.mro()

[abc.Model,
 aiida.orm.nodes.process.process.ProcessNode.Model,
 aiida.orm.utils.mixins.Sealable.Model,
 abc.Model,
 aiida.orm.nodes.node.Node.Model,
 abc.Model,
 aiida.orm.entities.Entity.Model,
 pydantic.main.BaseModel,
 object]

Again, everything appears to be working 🤷🏻‍♂️ But suspect nonetheless!

Feels like there's a "correct" approach that I'm just short of hitting. Could use another set of eyes on this.

@sphuber
Copy link
Contributor

sphuber commented Jan 15, 2024

@sphuber, @chrisjsewell I tested the pydantic version of this PR. Some tests are failing with

assert TypeError("Parameter.__init__() got an unexpected keyword argument 'is_attribute'") is None

This was already fixed, but probably pushed after you had rebased.

Anyhow, I rebases my work onto your branch and proceeded to work out model inheritance automation.

Consider the following:

class EntityChild(Entity, metaclass=EntityFieldMeta):
    class Model:
        attr: str = 'some_attr'

Note that Model does not explicitly inherit anything. This is ideal (not forcing anything on the user) but comes at a potential cost. My current implementation automates the model inheritance chain correctly in the sense that tests are passing (other than the ones already not passing on your branch) and the behavior is as expected.

Very nice. There is one thing that came to mind. During experimenting, I realized that sometimes a user may actually want to change inheritance on purpose. I don't have a concrete use case yet, but I wondered if it would be feasible, and if this approach (which would effectively prohibit it) would be too restrictive. In any case, this feature would not be used a whole lot (how many Data plugins exist out there) and if it is going to be used, people will anyway have to read documentation which can then clearly mention that they should subclass the right bases. But I am not sure of this, just thought I'd mention it.

However, pydantic raises the following warning on each triggering of my code in EntityFieldMeta:

UserWarning: Field name "attr" shadows an attribute in parent "Entity.Model";

This is due to the particular means in which I am constructing the Model. Instead of adding bases to the Model nested class (which seems to be forbidden), I create a new Model inheriting the explicitly defined Model and those of the Entity parents. I believe this results in inheriting attr before attr is redefined in the body of EntityChild.Model.

This seems to be just the knock-on effect of what you describe later where the dynamically created classes have duplicate base classes. This means the second time a base class is resolved, it redefines attr causing the warning. pydantic has this warning built in because pydantic.BaseModel defines some attributes itself, for example dict (at least in pydantic v1, this is now deprecated), and so if a user declared dict as an attribute, the original one would be shadowed.

I think then that solving the duplicate base models, will fix this warning. Would it not be possible to simply change __new__ to make sure the parent_models list is unique. So change the line

model = type('Model', (classdict['Model'], *set(parent_models), {})

Not sure if these parent_models are hashable though. If not, using set to make the list unique won't work and you will simply have to "manually" iterate over it and remove the duplicates.

@edan-bainglass edan-bainglass force-pushed the entity-fields branch 2 times, most recently from f6de54a to b2c7896 Compare March 14, 2024 11:29
@edan-bainglass
Copy link
Member Author

edan-bainglass commented Mar 14, 2024

@sphuber give it a look. I'll update the docs on approval.

Update

I see there are issues here with Docker. I take it this is due to the issue you mentioned with my aiida-core-dev PR?

@sphuber
Copy link
Contributor

sphuber commented Mar 14, 2024

I see there are issues here with Docker. I take it this is due to the issue you mentioned with my aiida-core-dev PR?

Looks very similar. Just the aiida-core-dev tests are failing. Just rebasing on latest main might fix it, since we merged the fix.

@edan-bainglass
Copy link
Member Author

Looks very similar. Just the aiida-core-dev tests are failing. Just rebasing on latest main might fix it, since we merged the fix.

I'm already on main 😭

@sphuber
Copy link
Contributor

sphuber commented Mar 14, 2024

Looks very similar. Just the aiida-core-dev tests are failing. Just rebasing on latest main might fix it, since we merged the fix.

I'm already on main 😭

Then you're up slack-alley 😅 Probably want to update the is_container_ready function in .docker/tests/conftest.py to print the output of the verdi status command. There must be a problem there, but I don't see how this would be related to your PR.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Mar 14, 2024

Then you're up slack-alley 😅 Probably want to update the is_container_ready function in .docker/tests/conftest.py to print the output of the verdi status command. There must be a problem there, but I don't see how this would be related to your PR.

Yeah, I don't see how either, but considering your PR is passing, as did Jason's fix, I'm concerned something in PR is clashing.

Update

See #6303 (comment)

@edan-bainglass
Copy link
Member Author

@sphuber may I proceed to document the changes to finalize this PR?

@edan-bainglass edan-bainglass requested a review from sphuber March 15, 2024 15:47
src/aiida/orm/fields.py Outdated Show resolved Hide resolved
@edan-bainglass edan-bainglass requested a review from sphuber March 16, 2024 07:18
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @edan-bainglass last few changes are all good. Just missed some things in the docs. If you can correct those and then squash all your changes in a single commit with a comprehensive commit message, we can merge this PR. Thanks a lot for all the effort!

docs/source/howto/query.rst Outdated Show resolved Hide resolved
docs/source/topics/data_types.rst Outdated Show resolved Hide resolved
docs/source/topics/data_types.rst Outdated Show resolved Hide resolved
@edan-bainglass
Copy link
Member Author

Thanks @sphuber. Waited for the green light before updating the docs. Will update in the morning. Thanks again 🙏

This commit first ensures that all query operations are supported in the
new syntax, including negation. The commit also separates the operations
by field type via subclassing. It introduces a uniform `orm.fields.add_field`
API as an abstraction of the type-dependent allocation of
`orm.fields.QbField` flavors.

A distinction is made in the API between database columns and attribute
fields via the `is_attribute` parameter. If set to `True` (the default), the field
is considered an attribute field and as such, `QbField().backend_key` will
return `'attribute.key'` in accordance with the `QueryBuilder` mechanics.
Note that _only_ in the case of attribute fields, `backend_key` may be
aliased via the `alias` parameter. This is done to avoid restrictions on
database field naming while maintaining a pythonic interface in the
frontend via the `key` parameter, which is validated as pythonic
(supporting dot notation).
@edan-bainglass
Copy link
Member Author

@sphuber I believe all is good now. On merge, please add an acknowledgement for @chrisjsewell work 🙂

@sphuber
Copy link
Contributor

sphuber commented Mar 18, 2024

@sphuber I believe all is good now. On merge, please add an acknowledgement for @chrisjsewell work 🙂

The first commit is still his right? I am planning to simply rebase merge, so the commits will remain attributed to Chris and yourself, respectively.

@sphuber sphuber merged commit 6dd8eca into aiidateam:main Mar 18, 2024
20 checks passed
@chrisjsewell
Copy link
Member

Cheers guys!

@edan-bainglass
Copy link
Member Author

@sphuber I neglected to comment on the backend mapping in the commit message ((and sufficiently document the feature). Follow up PR 🤷🏻‍♂️

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.

3 participants