ci: run python tests by abhizer · Pull Request #1873 · feldera/feldera

abhizer · 2024-06-14T15:58:08Z

Is this a user-visible change (yes/no): no

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

snkas

Looks good!

Only note I have is that there is only one environment variable which gets used for both internal variables. There could be two used, with the pipeline one falling back to the script one if it is not defined.

snkas · 2024-06-26T13:48:01Z

python/pyproject.toml

    "requests",
    "pandas",
    "typing-extensions",
+    "numpy<2",


Curiosity: which features of NumPy 2+ are non-compatible? As I see it has been recently released.

Yeah, we get an error with numpy 2+.

Seems to be a common thing:
https://stackoverflow.com/questions/78650222/valueerror-numpy-dtype-size-changed-may-indicate-binary-incompatibility-expec

snkas · 2024-06-26T13:49:47Z

python/tests/test_wireframes.py

+
+        if KAFKA_URL:
+            KAFKA_SERVER = KAFKA_URL
+            KAFKA_URL_FROM_PIPELINE = KAFKA_URL


Consistent naming of PIPELINE_TO_KAFKA_SERVER

snkas · 2024-06-26T13:51:47Z

python/tests/test_wireframes.py


+        # if the Kafka URL is set in the environment, use it
+        # useful for running tests in CI
+        KAFKA_URL = os.environ.get("KAFKA_URL")


I would say KAFKA_SERVER is more accurate as there is no scheme.

ryzhyk · 2024-06-26T17:55:24Z

Earthfile

            sleep 10 && \
            (./pipeline-manager --bind-address=0.0.0.0 --api-server-working-directory=/working-dir --compiler-working-directory=/working-dir --runner-working-directory=/working-dir --sql-compiler-home=/dbsp/sql-to-dbsp-compiler --dbsp-override-path=/dbsp --db-connection-string=postgresql://postgres:postgres@localhost:5432 --compilation-profile=unoptimized &) && \
            sleep 5 && \
+            docker run --name redpanda -p 9092:9092 --rm -itd docker.redpanda.com/vectorized/redpanda:v23.2.3 redpanda start --smp 2 && \


@gz , is there a downside to running the container this way as opposed to how we do it elsewhere in Earthfile, using docker compose, e.g.:

test-debezium-jdbc-sink: FROM earthly/dind:alpine COPY deploy/docker-compose.yml . COPY deploy/docker-compose-jdbc.yml . ENV FELDERA_VERSION=latest WITH DOCKER --pull docker.redpanda.com/vectorized/redpanda:v23.2.3 \ --pull debezium/example-postgres:2.3 \ --load ghcr.io/feldera/pipeline-manager:latest=+build-pipeline-manager-container \ --load ghcr.io/feldera/demo-container:latest=+build-demo-container \ --load ghcr.io/feldera/kafka-connect:latest=+build-kafka-connect-container RUN COMPOSE_HTTP_TIMEOUT=120 RUST_LOG=debug,tokio_postgres=info docker-compose -f docker-compose.yml -f docker-compose-jdbc.yml --profile debezium up --force-recreate --exit-code-from debezium-jdbc-demo END

hm not too familiar with earthly+docjer.. maybe @lalithsuresh knows why it was done that way in the file

I never dealt with Debezium, so I don't know. This does look odd. Worth rewriting it to be consistent with the other forms if it's quick to do.

gz

I guess my only question is why we need to test the python API with kafka in the loop

gz · 2024-06-27T15:24:04Z

Earthfile

    COPY +install-python-deps/wheels wheels
    COPY demo/demo_notebooks/requirements.txt requirements.txt
+    COPY --dir python ./
+    RUN pip install --upgrade pip


Any reason you need this? IMO this upgrades pip non-deterministically?

I think it's related to pyproject.toml not being (well) supported with out-of-the-box Ubuntu 22.04 (issue)
An alternative would be to either switch to legacy setup.py temporarily or to have both (will have to how that resolves though).

hm I thought we're already on 24.04 I swear I did the earthly upgrade at some point but it's not in main :( .. that's probably the right move for it

gz · 2024-06-27T15:25:46Z

Earthfile

            sleep 10 && \
            (./pipeline-manager --bind-address=0.0.0.0 --api-server-working-directory=/working-dir --compiler-working-directory=/working-dir --runner-working-directory=/working-dir --sql-compiler-home=/dbsp/sql-to-dbsp-compiler --dbsp-override-path=/dbsp --db-connection-string=postgresql://postgres:postgres@localhost:5432 --compilation-profile=unoptimized &) && \
            sleep 5 && \
+            docker run --name redpanda -p 9092:9092 --rm -itd docker.redpanda.com/vectorized/redpanda:v23.2.3 redpanda start --smp 2 && \


hm not too familiar with earthly+docjer.. maybe @lalithsuresh knows why it was done that way in the file

abhizer · 2024-06-28T13:26:45Z

I guess my only question is why we need to test the python API with kafka in the loop

There's a couple tests that require Kafka:

feldera/python/tests/test_wireframes.py

Line 183 in 8118183

def test_kafka(self):

And,

feldera/python/tests/test_wireframes.py

Line 281 in 8118183

def test_avro_format(self):

ryzhyk · 2024-06-27T21:01:09Z

Earthfile

+    COPY --dir python ./
+    RUN pip install --upgrade pip
    RUN pip wheel -r requirements.txt --wheel-dir=wheels
+    RUN pip wheel -r python/tests/requirements.txt --wheel-dir=wheels


I might be reading this wrong, but aren't we installing the same requirements again in the next action below?

We install it to the wheel directory here, and then next, install it via the wheel directory.
Just followed the pattern we use for requirements in demo_notebooks.

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

ci: run python tests

3cfe417

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

abhizer force-pushed the ci_py_test branch from df09941 to 3cfe417 Compare June 25, 2024 10:54

abhizer added 3 commits June 25, 2024 16:51

ci: install feldera-py, pytest in install-python-deps

f59f37c

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

ci: python: copy python dir before installing

3e354c2

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

ci: py: copy python/tests to pythontests

66f8ac7

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

abhizer mentioned this pull request Jun 25, 2024

Python API todos #1776

Closed

16 tasks

ci: run python tests

97b1a8c

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

abhizer marked this pull request as ready for review June 26, 2024 13:39

abhizer requested review from gz and snkas June 26, 2024 13:39

snkas approved these changes Jun 26, 2024

View reviewed changes

ryzhyk approved these changes Jun 26, 2024

View reviewed changes

gz approved these changes Jun 27, 2024

View reviewed changes

ryzhyk reviewed Jun 28, 2024

View reviewed changes

ci: py: don't run kafka tests

d085e6a

Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>

ryzhyk approved these changes Jun 28, 2024

View reviewed changes

Merge branch 'main' into ci_py_test

596ad35

abhizer merged commit 45add7c into main Jul 1, 2024

abhizer deleted the ci_py_test branch July 1, 2024 11:30

Conversation

abhizer commented Jun 14, 2024

Uh oh!

snkas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhizer commented Jun 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants