-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add TransformAndCatalog
flow
#720
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments
requirements.txt
Outdated
azure-identity==1.7.1 | ||
great-expectations==0.14.12 | ||
azure-identity>=1.10.0, <2.0.0 | ||
great-expectations>=0.15.50, <0.16.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you specifying a range of versions? wouldn't it be better to set version to specific one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed great-expectations to 0.15.50
tests/unit/tasks/test_luma.py
Outdated
|
||
def test_luma_ingest(): | ||
task = LumaIngest( | ||
credentials_secret="luma-dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of the time we specify credentials in uppercase
Args: | ||
dbt_project_path (str): The path to the dbt project (the directory containing | ||
the `dbt_project.yml` file). | ||
name (str): The name of the Flow. Defaults to "Transform and Catalog". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying flow name won't cause later issues? for example, it's easier to overwrite the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
docker/Dockerfile
Outdated
RUN pip install --upgrade pip | ||
RUN pip install -r requirements.txt | ||
RUN pip install -r requirements-dev.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are both requirements files needed or the dev one is used only for certain cases? if so you can add if statement for certain tag ie dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirements-dev
line is removed from Dockerfile
stream_output=True, | ||
).bind(flow=self) | ||
|
||
test_select = self.dbt_selects.get("test", run_select) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change the name from "test" to sth different, test suggests that it's something just fore testing not production
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was migrated from 2.0
. I will ask how it can be changed.
from prefect.utilities.logging import get_logger | ||
from prefect.utilities.tasks import defaults_from_attrs | ||
|
||
from viadot.task_utils import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to avoid using *?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an issue with the circular import
@Rafalz13 I think we also need the |
TransformAndCatalog
flow
Hm I didn't udnerstand that. You can still add it to the base class; then only the custom sources will need to be modified to use the util function (or just use the The way you did this, you need to modify all |
viadot/utils.py
Outdated
@@ -18,7 +20,7 @@ | |||
from .exceptions import APIError | |||
from .signals import SKIP | |||
|
|||
logger = logging.get_logger(__name__) | |||
logger = get_logger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be a Prefect logger? These utils are a the viadot level; Prefect utils are in a separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
It is a need to use DBT in viadot 1.0. DBTTask and CloneRepo tasks are migrated from viadot 2.0. Also, TransformAndCatalog flow is built using viadot 1.0 structure.
Importance
To be able to use the DBT using viadot 1.0.
Checklist
This PR:
CONTRIBUTING.md
CHANGELOG.md