chore: Generate environments for each individual test based on its markers/fixtures #2648

pyalex · 2022-05-06T22:27:17Z

Signed-off-by: Oleksii Moskalenko moskalenko.alexey@gmail.com

What this PR does / why we need it:

Test environment is being generated individually for each integration test based on its markers and/or requested fixtures. This allows us to drop some unnecessary use cases (eg, looping various online stores against test_historical_features) and shorten total run time.

The main change is pytest_generate_tests function in conftest.py that replaces static declaration of repo configs. It generates environments for each test and retrieves them from configs cache, so that environments could be shared between different tests.

Which issue(s) this PR fixes:

Fixes #

codecov-commenter · 2022-05-06T22:33:14Z

Codecov Report

Merging #2648 (900df33) into master (640ff12) will decrease coverage by 21.65%.
The diff coverage is 53.73%.

@@             Coverage Diff             @@
##           master    #2648       +/-   ##
===========================================
- Coverage   80.55%   58.89%   -21.66%     
===========================================
  Files         163      164        +1     
  Lines       13815    13761       -54     
===========================================
- Hits        11128     8105     -3023     
- Misses       2687     5656     +2969

Flag	Coverage Δ
integrationtests	`?`
unittests	`58.89% <53.73%> (+0.28%)`	⬆️

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

Impacted Files	Coverage Δ
sdk/python/feast/feature_logging.py	`49.45% <0.00%> (-39.44%)`	⬇️
...s/contrib/spark_offline_store/tests/data_source.py	`40.42% <ø> (ø)`
...thon/feast/infra/online_stores/contrib/postgres.py	`32.69% <0.00%> (ø)`
sdk/python/feast/infra/online_stores/datastore.py	`39.90% <0.00%> (-10.33%)`	⬇️
...tegration/feature_repos/universal/feature_views.py	`36.11% <ø> (-45.84%)`	⬇️
...s/contrib/trino_offline_store/tests/data_source.py	`47.05% <25.00%> (ø)`
...ests/integration/e2e/test_python_feature_server.py	`32.39% <25.00%> (-67.61%)`	⬇️
sdk/python/feast/feature_server.py	`34.28% <33.33%> (-38.85%)`	⬇️
...ts/integration/feature_repos/repo_configuration.py	`55.17% <37.50%> (-34.28%)`	⬇️
...on/tests/integration/e2e/test_go_feature_server.py	`41.66% <42.85%> (-56.61%)`	⬇️
... and 102 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 640ff12...900df33. Read the comment docs.

achals · 2022-05-09T17:46:34Z

go/internal/feast/server/logging/memorybuffer.go

@@ -78,7 +78,7 @@ func (b *MemoryBuffer) getArrowSchema() (*arrow.Schema, error) {
 		fields = append(fields, arrow.Field{Name: featureName, Type: arrowType})
 		fields = append(fields, arrow.Field{
 			Name: fmt.Sprintf("%s__timestamp", featureName),
-			Type: arrow.FixedWidthTypes.Timestamp_s})
+			Type: arrow.FixedWidthTypes.Timestamp_ms})


Seems like this should in a separate PR, unless it's fixing some bug.

This is reverted

achals · 2022-05-09T17:48:01Z

sdk/python/feast/infra/offline_stores/contrib/postgres_repo_configuration.py

+
+AVAILABLE_OFFLINE_STORES = [("local", PostgreSQLDataSourceCreator)]
+
+AVAILABLE_ONLINE_STORES = {"postgres": (POSTGRES_ONLINE_CONFIG, None)}


Can't we use the AVAILABLE_OFFLINE_STORES here as well? POSTGRES_ONLINE_CONFIG assumes you have a local version of postgres running and a containerized version is better.

achals · 2022-05-09T17:51:38Z

sdk/python/tests/conftest.py

+        if "universal_offline_stores" in markers:
+            offline_stores = AVAILABLE_OFFLINE_STORES
+        else:
+            # default offline store for testing online store dimension
+            offline_stores = [("local", FileDataSourceCreator)]
+
+        online_stores = None
+        if "universal_online_stores" in markers:


Should we get rid of the universal flag? since it doesn't seem like the online and offline versions are more versatile?

So I need a name here that would represent that specific tests works with all offline stores (universal_offline_stores mark currently) or all online stores (universal_online_stores mark). Any ideas for a better name?

I meant the pytest.mark.universal flag (since it seems like this the universal_online_stores and universal_offline_stores together would represent the same thing that current flag is supposed to represent).

Got it. Removed universal.

achals · 2022-05-09T17:55:35Z

sdk/python/tests/integration/registration/test_universal_types.py

@@ -8,63 +8,48 @@
 import pyarrow as pa
 import pytest

+from feast import Entity


nit, from feast.entity import Entity (for tests)

achals · 2022-05-09T19:30:50Z

sdk/python/feast/infra/offline_stores/contrib/postgres_repo_configuration.py

+POSTGRES_ONLINE_CONFIG = {
+    "type": "postgres",
+    "host": "localhost",
+    "port": "5432",
+    "database": "postgres",
+    "db_schema": "feature_store",
+    "user": "postgres",
+    "password": "docker",
+}


My PR changes this, #2650

Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

pyalex · 2022-05-11T00:03:48Z

sdk/python/feast/infra/online_stores/datastore.py

    query = client.query(kind="Row", ancestor=key)
-    while True:


infinite looping here might hang the whole testing flow

achals · 2022-05-11T05:31:20Z

sdk/python/tests/conftest.py

+_config_cache = {}
+
+
+def pytest_generate_tests(metafunc):


Can you add some docs to how this pytest_generate_tests thingy works? It's the first time I've seen it used and seems extremely powerful

Added comment.

achals · 2022-05-11T05:32:27Z

sdk/python/tests/conftest.py

+        if "universal_offline_stores" in markers:
+            offline_stores = AVAILABLE_OFFLINE_STORES
+        else:
+            # default offline store for testing online store dimension
+            offline_stores = [("local", FileDataSourceCreator)]
+
+        online_stores = None
+        if "universal_online_stores" in markers:


I meant the pytest.mark.universal flag (since it seems like this the universal_online_stores and universal_offline_stores together would represent the same thing that current flag is supposed to represent).

feast-ci-bot · 2022-05-11T05:36:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, pyalex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [achals]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

achals · 2022-05-11T17:25:58Z

/lgtm

Thanks for addressing! This looks great.

pyalex changed the title ~~chore: Generate environments for each individual test based on their markers~~ [WIP] chore: Generate environments for each individual test based on their markers May 6, 2022

feast-ci-bot added do-not-merge/work-in-progress size/XL labels May 6, 2022

pyalex added ok-to-test and removed size/XL labels May 6, 2022

feast-ci-bot added the size/XL label May 6, 2022

pyalex added ok-to-test and removed ok-to-test labels May 6, 2022

pyalex force-pushed the simplify-tests branch 2 times, most recently from 134251c to b58c052 Compare May 9, 2022 18:56

achals reviewed May 9, 2022

View reviewed changes

pyalex force-pushed the simplify-tests branch from ae071e3 to 089bf76 Compare May 9, 2022 23:23

feast-ci-bot added size/XXL and removed size/XL labels May 9, 2022

pyalex mentioned this pull request May 10, 2022

fix: Feature logging to Redshift is broken #2655

Merged

pyalex force-pushed the simplify-tests branch from 2a28de7 to 4bfb8cf Compare May 10, 2022 22:26

feast-ci-bot added size/XL and removed size/XXL labels May 10, 2022

pyalex added ok-to-test and removed ok-to-test labels May 10, 2022

pyalex force-pushed the simplify-tests branch from 4bfb8cf to 15d22f0 Compare May 10, 2022 22:33

generating test environments bases on test markers and fixtures

155feee

Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

pyalex force-pushed the simplify-tests branch from 15d22f0 to 155feee Compare May 11, 2022 00:00

pyalex commented May 11, 2022

View reviewed changes

pyalex changed the title ~~[WIP] chore: Generate environments for each individual test based on their markers~~ chore: Generate environments for each individual test based on their markers May 11, 2022

feast-ci-bot removed the do-not-merge/work-in-progress label May 11, 2022

pyalex changed the title ~~chore: Generate environments for each individual test based on their markers~~ chore: Generate environments for each individual test based on its markers/fixtures May 11, 2022

achals approved these changes May 11, 2022

View reviewed changes

feast-ci-bot added the approved label May 11, 2022

remove "universal" marker

b5e40c4

Signed-off-by: Oleksii Moskalenko <moskalenko.alexey@gmail.com>

feast-ci-bot assigned achals May 11, 2022

feast-ci-bot added the lgtm label May 11, 2022

pyalex added ok-to-test and removed ok-to-test labels May 11, 2022

feast-ci-bot merged commit d4b0b1a into feast-dev:master May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Generate environments for each individual test based on its markers/fixtures #2648

chore: Generate environments for each individual test based on its markers/fixtures #2648

pyalex commented May 6, 2022 •

edited

Loading

codecov-commenter commented May 6, 2022 •

edited

Loading

achals May 9, 2022

pyalex May 10, 2022

achals May 9, 2022

pyalex May 10, 2022

achals May 9, 2022

pyalex May 10, 2022

achals May 11, 2022

pyalex May 11, 2022

achals May 9, 2022

pyalex May 10, 2022

achals May 9, 2022

pyalex May 10, 2022

pyalex May 11, 2022

achals May 11, 2022

pyalex May 11, 2022

achals May 11, 2022

feast-ci-bot commented May 11, 2022

achals commented May 11, 2022


		AVAILABLE_OFFLINE_STORES = [("local", PostgreSQLDataSourceCreator)]

		AVAILABLE_ONLINE_STORES = {"postgres": (POSTGRES_ONLINE_CONFIG, None)}

		_config_cache = {}


		def pytest_generate_tests(metafunc):

chore: Generate environments for each individual test based on its markers/fixtures #2648

chore: Generate environments for each individual test based on its markers/fixtures #2648

Conversation

pyalex commented May 6, 2022 • edited Loading

codecov-commenter commented May 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feast-ci-bot commented May 11, 2022

achals commented May 11, 2022

pyalex commented May 6, 2022 •

edited

Loading

codecov-commenter commented May 6, 2022 •

edited

Loading