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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ on:
- master
- 'push-action/**'

env:
PYTEST_ADDOPTS: "--color=yes"

# Cancel running workflows when additional changes are pushed
# https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-a-fallback-value
concurrency:
Expand Down Expand Up @@ -162,7 +165,7 @@ jobs:
ports:
- 5432:5432
elasticsearch:
image: elasticsearch:7.17.1
image: elasticsearch:7.17.7
ports:
- 9200:9200
- 9300:9300
Expand Down
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ repos:
- id: trailing-whitespace
args: [--markdown-linebreak-ext=md]

- repo: https://gitlab.com/pycqa/flake8
rev: '3.9.2'
- repo: https://github.com/pycqa/flake8
rev: '5.0.4'
hooks:
- id: flake8

Expand Down
2 changes: 1 addition & 1 deletion INSTALL.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ These commands should be run from a local optimade-python-tools directory.
The following command starts a local Elasticsearch v6 instance, runs the test suite, then stops and deletes the containers (required as the tests insert some data):

```shell
docker run -d --name elasticsearch_test -p 9200:9200 -p 9300:9300 -e "discovery.type=single-node" -e "xpack.security.enabled=false" elasticsearch:7.17.1 \
docker run -d --name elasticsearch_test -p 9200:9200 -p 9300:9300 -e "discovery.type=single-node" -e "xpack.security.enabled=false" elasticsearch:7.17.7 \
JPBergsma marked this conversation as resolved.
Show resolved Hide resolved
&& sleep 10 \
&& OPTIMADE_DATABASE_BACKEND="elastic" py.test; \
docker container stop elasticsearch_test; docker container rm elasticsearch_test
Expand Down
2 changes: 1 addition & 1 deletion optimade/server/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ class ServerConfig(BaseSettings):
description="Which database backend to use out of the supported backends.",
)

elastic_hosts: Optional[List[Dict]] = Field(
elastic_hosts: Optional[Union[str, List[str], Dict, List[Dict]]] = Field(
None, description="Host settings to pass through to the `Elasticsearch` class."
)

Expand Down
18 changes: 9 additions & 9 deletions optimade/server/entry_collections/elasticsearch.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ def create_optimade_index(self) -> None:
]["properties"].pop(field)
properties["id"] = {"type": "keyword"}
body["mappings"]["properties"] = properties
self.client.indices.create(index=self.name, body=body, ignore=400)
self.client.indices.create(index=self.name, ignore=400, **body)

LOGGER.debug(f"Created Elastic index for {self.name!r} with body {body}")
LOGGER.debug(f"Created Elastic index for {self.name!r} with parameters {body}")

@property
def predefined_index(self) -> Dict[str, Any]:
Expand All @@ -94,7 +94,8 @@ def create_elastic_index_from_mapper(
fields: The list of fields to use in the index.

Returns:
The `body` parameter to pass to `client.indices.create(..., body=...)`.
The parameters to pass to `client.indices.create(...)` (previously
the 'body' parameters).

"""
properties = {
Expand Down Expand Up @@ -135,15 +136,14 @@ def get_id(item):

bulk(
self.client,
[
(
{
"_index": self.name,
"_id": get_id(item),
"_type": "_doc",
"_source": item,
}
for item in data
],
),
)

def _run_db_query(
Expand Down Expand Up @@ -179,9 +179,9 @@ def _run_db_query(
for field, sort_dir in criteria.get("sort", {})
]
if not elastic_sort:
elastic_sort = {
self.resource_mapper.get_backend_field("id"): {"order": "asc"}
}
elastic_sort = [
{self.resource_mapper.get_backend_field("id"): {"order": "asc"}}
]

search = search.sort(*elastic_sort)

Expand Down
11 changes: 0 additions & 11 deletions pytest.ini

This file was deleted.

1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
elasticsearch==7.17.7
elasticsearch-dsl==7.4.0
email_validator==1.3.0
fastapi==0.86.0
Expand Down
16 changes: 16 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,19 @@ ignore = E251,E252,E501,W503

[flake8]
ignore = E402,W503,E501,E203

[tool:pytest]
JPBergsma marked this conversation as resolved.
Show resolved Hide resolved
filterwarnings =
error
ignore:.*parameter is deprecated for the.*:DeprecationWarning
ignore:.*read_text is deprecated.*:DeprecationWarning
ignore:.*open_text is deprecated.*:DeprecationWarning
ignore:.*SixMetaPathImporter.*:ImportWarning
ignore:.*PY_SSIZE_T_CLEAN will be required for.*:DeprecationWarning
ignore:.*not found, cannot convert structure.*:
ignore:.*will default to setting mass to 1\.0\.:
ignore:.*is missing fields.*which are required if.*:
ignore:.*the imp module is deprecated in favour of importlib.*:DeprecationWarning
ignore:.*has an unrecognised prefix.*:
testpaths = tests
addopts = -rs
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

mongo_deps = ["pymongo>=3.12.1,<5", "mongomock~=4.1"]
server_deps = [
"uvicorn~=0.19",
Expand Down Expand Up @@ -101,7 +101,7 @@
python_requires=">=3.8",
install_requires=[
"lark~=1.1",
"fastapi~=0.86",
"fastapi~=0.86.0",
"pydantic~=1.10,>=1.10.2",
"email_validator~=1.2",
"requests~=2.28",
Expand Down
5 changes: 3 additions & 2 deletions tests/adapters/structures/test_structures.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
"""Test Structure adapter"""
import pytest

from optimade.adapters import Structure
from optimade.models import StructureResource

try:
import aiida # noqa: F401
import ase # noqa: F401
import jarvis # noqa: F401
import numpy # noqa: F401
import pymatgen # noqa: F401
import jarvis # noqa: F401
except ImportError:
all_modules_found = False
else:
Expand All @@ -34,6 +33,7 @@ def test_convert(structure):
"""Test convert() works
Choose currently known entry type - must be updated if no longer available.
"""
pytest.importorskip("numpy")
if not structure._type_converters:
pytest.fail("_type_converters is seemingly empty. This should not be.")

Expand Down Expand Up @@ -102,6 +102,7 @@ def test_getattr_order(structure):
def test_no_module_conversion(structure):
"""Make sure a warnings is raised and None is returned for conversions with non-existing modules"""
import importlib

from optimade.adapters.warnings import AdapterPackageNotFound

CONVERSION_MAPPING = {
Expand Down
5 changes: 1 addition & 4 deletions tests/filtertransformers/test_elasticsearch.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import pytest


elasticsearch_dsl = pytest.importorskip(
"elasticsearch_dsl", reason="No ElasticSearch installation, skipping tests..."
)

from optimade.filterparser import LarkParser
from optimade.filtertransformers.elasticsearch import (
ElasticTransformer,
)
from optimade.filtertransformers.elasticsearch import ElasticTransformer


@pytest.fixture
Expand Down
8 changes: 5 additions & 3 deletions tests/filtertransformers/test_mongo.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from optimade.filterparser import LarkParser
from optimade.server.exceptions import BadRequest
from optimade.server.warnings import UnknownProviderProperty


class TestMongoTransformer:
Expand Down Expand Up @@ -442,9 +443,10 @@ def test_other_provider_fields(self, mapper):

t = MongoTransformer(mapper=mapper("StructureMapper"))
p = LarkParser(version=self.version, variant=self.variant)
assert t.transform(p.parse("_other_provider_field > 1")) == {
"_other_provider_field": {"$gt": 1}
}
with pytest.warns(UnknownProviderProperty):
assert t.transform(p.parse("_other_provider_field > 1")) == {
"_other_provider_field": {"$gt": 1}
}

def test_not_implemented(self):
"""Test that list properties that are currently not implemented
Expand Down
26 changes: 13 additions & 13 deletions tests/models/test_structures.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
# pylint: disable=no-member
import pytest
import itertools

from pydantic import ValidationError

from optimade.models.structures import StructureResource, CORRELATED_STRUCTURE_FIELDS
import pytest
from optimade.models.structures import CORRELATED_STRUCTURE_FIELDS, StructureResource
from optimade.server.warnings import MissingExpectedField

from pydantic import ValidationError

MAPPER = "StructureMapper"

Expand Down Expand Up @@ -63,13 +61,14 @@ def test_more_good_structures(good_structures, mapper):

def test_bad_structures(bad_structures, mapper):
"""Check badly formed structures"""
for index, structure in enumerate(bad_structures):
# This is for helping devs finding any errors that may occur
print(
f"Trying structure number {index}/{len(bad_structures)} from 'test_bad_structures.json'"
)
with pytest.raises(ValidationError):
StructureResource(**mapper(MAPPER).map_back(structure))
with pytest.warns(MissingExpectedField):
for index, structure in enumerate(bad_structures):
# This is for helping devs finding any errors that may occur
print(
f"Trying structure number {index}/{len(bad_structures)} from 'test_bad_structures.json'"
)
with pytest.raises(ValidationError):
StructureResource(**mapper(MAPPER).map_back(structure))


deformities = (
Expand Down Expand Up @@ -194,7 +193,8 @@ def test_structure_fatal_deformities(good_structure, deformity):
import re

if deformity is None:
return StructureResource(**good_structure)
StructureResource(**good_structure)
return

deformity, message = deformity
good_structure["attributes"].update(deformity)
Expand Down
23 changes: 12 additions & 11 deletions tests/server/middleware/test_query_param.py
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
"""Test EntryListingQueryParams middleware"""
import pytest

from optimade.server.exceptions import BadRequest
from optimade.server.middleware import EnsureQueryParamIntegrity
from optimade.server.warnings import FieldValueNotRecognized


def test_wrong_html_form(check_error_response, both_clients):
"""Using a parameter without equality sign `=` or values should result in a `400 Bad Request` response"""
from optimade.server.query_params import EntryListingQueryParams

for valid_query_parameter in EntryListingQueryParams().__dict__:
request = f"/structures?{valid_query_parameter}"
with pytest.raises(BadRequest):
check_error_response(
request,
expected_status=400,
expected_title="Bad Request",
expected_detail="A query parameter without an equal sign (=) is not supported by this server",
server=both_clients,
)
with pytest.warns(FieldValueNotRecognized):
for valid_query_parameter in EntryListingQueryParams().__dict__:
request = f"/structures?{valid_query_parameter}"
with pytest.raises(BadRequest):
check_error_response(
request,
expected_status=400,
expected_title="Bad Request",
expected_detail="A query parameter without an equal sign (=) is not supported by this server",
server=both_clients,
)


def test_wrong_html_form_one_wrong(check_error_response, both_clients):
Expand Down
48 changes: 27 additions & 21 deletions tests/server/test_client.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
from pathlib import Path
import json

import pytest
import httpx
from functools import partial
from pytest_httpx import HTTPXMock
from pathlib import Path


from optimade.client import OptimadeClient
from optimade.client.cli import _get
import pytest
from optimade.server.warnings import MissingExpectedField

try:
from optimade.client import OptimadeClient
except ImportError as exc:
pytest.skip(str(exc), allow_module_level=True)

TEST_URLS = [
"https://example.com",
Expand All @@ -20,6 +21,8 @@

@pytest.fixture(scope="function")
def httpx_mocked_response(httpx_mock, client):
import httpx

def httpx_mock_response(client, request: httpx.Request):
response = client.get(str(request.url))
return httpx.Response(status_code=response.status_code, json=response.json())
Expand All @@ -29,7 +32,7 @@ def httpx_mock_response(client, request: httpx.Request):


@pytest.mark.parametrize("use_async", [False])
def test_client_endpoints(httpx_mocked_response: HTTPXMock, use_async):
JPBergsma marked this conversation as resolved.
Show resolved Hide resolved
def test_client_endpoints(httpx_mocked_response, use_async):

filter = ""

Expand Down Expand Up @@ -76,19 +79,20 @@ def test_filter_validation(use_async):

@pytest.mark.parametrize("use_async", [False])
def test_client_response_fields(httpx_mocked_response, use_async):
cli = OptimadeClient(base_urls=[TEST_URL], use_async=use_async)
results = cli.get(response_fields=["chemical_formula_reduced"])
for d in results["structures"][""][TEST_URL]["data"]:
assert "chemical_formula_reduced" in d["attributes"]
assert len(d["attributes"]) == 1

results = cli.get(
response_fields=["chemical_formula_reduced", "cartesian_site_positions"]
)
for d in results["structures"][""][TEST_URL]["data"]:
assert "chemical_formula_reduced" in d["attributes"]
assert "cartesian_site_positions" in d["attributes"]
assert len(d["attributes"]) == 2
with pytest.warns(MissingExpectedField):
cli = OptimadeClient(base_urls=[TEST_URL], use_async=use_async)
results = cli.get(response_fields=["chemical_formula_reduced"])
for d in results["structures"][""][TEST_URL]["data"]:
assert "chemical_formula_reduced" in d["attributes"]
assert len(d["attributes"]) == 1

results = cli.get(
response_fields=["chemical_formula_reduced", "cartesian_site_positions"]
)
for d in results["structures"][""][TEST_URL]["data"]:
assert "chemical_formula_reduced" in d["attributes"]
assert "cartesian_site_positions" in d["attributes"]
assert len(d["attributes"]) == 2


@pytest.mark.parametrize("use_async", [False])
Expand All @@ -113,6 +117,8 @@ def test_client_sort(httpx_mocked_response, use_async):

@pytest.mark.parametrize("use_async", [False])
def test_command_line_client(httpx_mocked_response, use_async, capsys):
from optimade.client.cli import _get

args = dict(
use_async=use_async,
filter=['elements HAS "Ag"'],
Expand Down