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

dbt-materialize: v1.8.0 migration #27011

Merged

Conversation

bobbyiliev
Copy link
Contributor

@bobbyiliev bobbyiliev commented May 9, 2024

Motivation

Fixes #26226.

Initial PR to update the required dependencies for the dbt v1.8.0 migration.

@bobbyiliev bobbyiliev changed the title Dbt materialize v1.8.0 migration [WIP] Dbt materialize v1.8.0 migration May 9, 2024
@bobbyiliev bobbyiliev changed the title [WIP] Dbt materialize v1.8.0 migration [WIP] dbt-materialize: v1.8.0 migration May 9, 2024
@morsapaes
Copy link
Contributor

@bobbyiliev, to fix linting, you can run: ../../bin/pyfmt; which will ✨ automatically✨ re-format any files. You then just need to push the changes in!

@morsapaes morsapaes force-pushed the dbt-materialize-v1.8.0-migration branch from 3107508 to 99e88dd Compare May 14, 2024 20:26
@morsapaes
Copy link
Contributor

@nrainer-materialize, the tests are failing on what seems to be a Docker-related issue; do you have a hunch about what's going on?

$ docker compose run -eDBT_HOST=materialized -eKAFKA_ADDR=redpanda:9092 -eSCHEMA_REGISTRY_URL=http://schema-registry:8081 dbt pytest dbt-materialize/tests
time="2024-05-14T20:33:01Z" level=error msg="error waiting for container: context canceled"
Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "pytest": executable file not found in $PATH: unknown

@nrainer-materialize
Copy link
Contributor

@nrainer-materialize, the tests are failing on what seems to be a Docker-related issue; do you have a hunch about what's going on?

$ docker compose run -eDBT_HOST=materialized -eKAFKA_ADDR=redpanda:9092 -eSCHEMA_REGISTRY_URL=http://schema-registry:8081 dbt pytest dbt-materialize/tests
time="2024-05-14T20:33:01Z" level=error msg="error waiting for container: context canceled"
Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "pytest": executable file not found in $PATH: unknown

Well, it says that it cannot find pytest in the Docker image. While I first could not find any suspicious changes in the pull request, I am wondering now if the issue could be related to the removal of the following code (though, just guessing):

    extras_require={
        "dev": ["dbt-tests-adapter~=1.7.0"],
    },

Does this dependency maybe ship with pytest?

@morsapaes
Copy link
Contributor

That was my first hunch, but dbt-tests-adapter is now bundled in dbt-adapters. A previous commit from Bobby included the dependency explicitly, but was failing with the same error. It makes sense that this is expecting pytest to be installed with the connector, though!

@nrainer-materialize
Copy link
Contributor

nrainer-materialize commented May 15, 2024

That was my first hunch, but dbt-tests-adapter is now bundled in dbt-adapters. A previous commit from Bobby included the dependency explicitly, but was failing with the same error. It makes sense that this is expecting pytest to be installed with the connector, though!

Well, it is the Docker image, which does no longer include pytest.

This build uses materialize/dbt-materialize:mzbuild-XCEFLV6YVDL2A4UB7KZCNNFNQ2TJID2G. A build on main uses materialize/dbt-materialize:mzbuild-XXTN55FJRPU2CVG5GVWMQK2JI6EPGFBD.

This branch:

nrainer@mz-nrainer mz-repo % docker run -it materialize/dbt-materialize:mzbuild-XCEFLV6YVDL2A4UB7KZCNNFNQ2TJID2G bash
root@6e752dc26bef:/# ls
bin  boot  dbt-materialize  dev  etc  home  lib  media	mnt  opt  proc	root  run  sbin  srv  sys  tmp	usr  var
root@6e752dc26bef:/# find . -name "pytest"

main:

nrainer@mz-nrainer ~ % docker run -it materialize/dbt-materialize:mzbuild-XXTN55FJRPU2CVG5GVWMQK2JI6EPGFBD bash
root@fead2752336c:/# ls
bin  boot  dbt-materialize  dev  etc  home  lib  media	mnt  opt  proc	root  run  sbin  srv  sys  tmp	usr  var
root@fead2752336c:/# find . -name "pytest"
./usr/local/bin/pytest
./usr/local/lib/python3.8/site-packages/pytest
root@fead2752336c:/# 

That is, if the Docker image is based on the changed setup file (which I assume), it has to do with the dependencies.

@nrainer-materialize
Copy link
Contributor

Try adding

RUN pip install pitest

to misc/dbt-materialize/Dockerfile.

@morsapaes
Copy link
Contributor

morsapaes commented May 15, 2024

____________ ERROR collecting tests/adapter/test_relation_types.py _____________
ImportError while importing test module '/dbt-materialize/tests/adapter/test_relation_types.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
usr/local/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
dbt-materialize/tests/adapter/test_relation_types.py:19: in <module>
    from fixtures import (
dbt-materialize/tests/adapter/fixtures.py:16: in <module>
    from dbt.tests.adapter.hooks import test_model_hooks as core_base
E   ModuleNotFoundError: No module named 'dbt.tests.adapter'

This confirms that we're just not picking up the test dependencies from dbt-adapters (or, /dbt-test-adapters) correctly. @colin-rogers-dbt, it feels like we're missing some basic nuance that is probably straightforward for you to single out; mind giving this a look? Adding pytest to the Dockerfile and adding the dependency to the dbt-tests-adapter subdirectory seems to have done it!

@bobbyiliev bobbyiliev force-pushed the dbt-materialize-v1.8.0-migration branch from 0917e28 to 8f64cc7 Compare May 15, 2024 12:25
@bobbyiliev bobbyiliev force-pushed the dbt-materialize-v1.8.0-migration branch from 70f12db to a89cf9e Compare May 16, 2024 18:03
@bobbyiliev bobbyiliev marked this pull request as ready for review May 20, 2024 08:31
@bobbyiliev bobbyiliev changed the title [WIP] dbt-materialize: v1.8.0 migration dbt-materialize: v1.8.0 migration May 20, 2024
@morsapaes morsapaes force-pushed the dbt-materialize-v1.8.0-migration branch from d11df75 to 7a91aab Compare May 20, 2024 09:12
@morsapaes morsapaes force-pushed the dbt-materialize-v1.8.0-migration branch 2 times, most recently from 70ab216 to 26d594b Compare May 20, 2024 21:42
@morsapaes morsapaes force-pushed the dbt-materialize-v1.8.0-migration branch from 26d594b to 0d16455 Compare May 20, 2024 21:48
@morsapaes
Copy link
Contributor

Great job on the upgrade, @bobbyiliev! There's a couple of minor things we can follow-up on, but I'm confident about rolling out the release.

@bobbyiliev bobbyiliev merged commit d285c1f into MaterializeInc:main May 23, 2024
12 checks passed
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.

upgrade to support dbt-core v1.8.0
3 participants