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

Improve test diagnostics and fix deprecated Elasticsearch calls #1373

Merged
merged 4 commits into from Nov 18, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Oct 25, 2022

This PR does the following:

  • treat deprecation warnings as errors unless they are explicitly ignored
  • enables coloured outputs in CI with pytest
  • moves pytest config to setup.cfg instead of pytest.ini
  • attempts to filter more warnings out of pytest output (this is still not working quite as I'd hoped)
  • Fixes some deprecated Elasticsearch calls, and pins the version of the underlying elasticsearch dependency (not elasticsearch_dsl)
  • Pins FastAPI to 0.86 in setup.py due to issues with 0.87

Hopefully now we will get proper test errors for important warnings from e.g. dependencies.

Closes #1377 (I think) and #1388

@ml-evs ml-evs force-pushed the ml-evs/tweak_pytest branch 2 times, most recently from bcf5b7c to e75333e Compare November 15, 2022 10:44
- Enable coloured output from pytest in Actions

- Move pytest config to setup.cfg

- Skip client tests if deps not present

- Add -rs to default pytest config

- Use GitHub flake8 repo in pre-commit
@ml-evs ml-evs force-pushed the ml-evs/tweak_pytest branch 2 times, most recently from 506dbc3 to bb4ecd2 Compare November 15, 2022 11:06
@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #1373 (41eed44) into master (2394476) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1373   +/-   ##
=======================================
  Coverage   91.46%   91.46%           
=======================================
  Files          72       72           
  Lines        4369     4369           
=======================================
  Hits         3996     3996           
  Misses        373      373           
Flag Coverage Δ
project 91.46% <100.00%> (ø)
validator 91.52% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/server/config.py 93.54% <100.00%> (ø)
optimade/server/entry_collections/elasticsearch.py 97.53% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

- Use recommend (i.e., not deprecated) elasticsearch create index call

- Do not use deprecated body parameter in elasticsearch

- Add explicit dependency on elasticsearch low-level Python API

- Bump ES service version in GH actions

- Bump ES version in docs
- Tweak deprecation skip

- Ignore py311 deprecation warning from aiida internals

- Pin fastapi more aggressively and tweak warnings
@ml-evs ml-evs marked this pull request as ready for review November 15, 2022 17:15
@ml-evs ml-evs added enhancement New feature or request transformer/Elasticsearch Related to filter transformer for Elasticsearch tests Related to tests CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) labels Nov 15, 2022
@ml-evs ml-evs changed the title Improve diagnostic output from pytest in CI Improve test diagnostics and fix deprecated Elasticsearch calls Nov 15, 2022
INSTALL.md Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@@ -18,7 +18,7 @@

# Dependencies
# Server minded
elastic_deps = ["elasticsearch-dsl~=7.4,<8.0"]
elastic_deps = ["elasticsearch-dsl~=7.4,<8.0", "elasticsearch~=7.17"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to explicitly define the elastic search version?
elasticsearch-dsl depends on elasticsearch, so it should be installed without specifying it directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, yes. Different versions of elasticsearch are introducing different functionality that elasticsearch-dsl is making use of in different ways. Currently DLS is making deprecated calls to the underlying package without pinning the version, so if we want to avoid deprecation warnings ourselves then I think this is the only choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that we get deprecation warnings because the elasticsearch version is too old ?
And that elasticsearch it is not automatically updated when we update elasticsearch-dsl ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We get them because elasticsearch_dsl is using the Python API defined in elasticsearch (the Python package) but is not completely up to date itself (and does not pin a specific version as it is a library).

Copy link
Contributor

Choose a reason for hiding this comment

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

So does Elasticsearch-dsl give you a deprecation warning when you use an older version(e.g. 7.1) of Elasticsearch ?
Anyway, I do not think this is a big enough issue to hold back this PR, so I have approved it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Python client elasticsearch (not the database) follows the same version convention as the database, but elasticsearch_dsl (the higher-level interface) just uses the major version to say they support v7 of the database. We were directly making calls to the low-level elasticsearch Python API (without adding it as an explicit dependency, until this PR) that have been deprecated (fixed in this PR), but the DSL is still making similar deprecated calls that we cannot fix at our end - I have added these to the pytest ignore list. As elasticsearch-dsl does not tightly pin the version of elasticsearch Python API, there is a chance that those calls get removed in a later version, which would also break our package unless we pin it.

@ml-evs ml-evs merged commit b21d894 into master Nov 18, 2022
@ml-evs ml-evs deleted the ml-evs/tweak_pytest branch November 18, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) enhancement New feature or request tests Related to tests transformer/Elasticsearch Related to filter transformer for Elasticsearch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch pytest oneliner in the docs is no longer working
2 participants