Update metricflow dependencies
#1246
Conversation
plypaul
force-pushed
the
p--py312--12
branch
4 times, most recently
from
144d0bd to
ea06513
Compare
There was a problem hiding this comment.
I'm not convinced this will run unless the user happens to have graphviz installed, since metricflow_semantics uses it.
We only access this module in the CLI package so maybe we move it over there along with the graphviz dependency. Or else we can remove the graphviz rendering altogether, I don't think anybody's using it. It's cool and all, but eh.
| PyYAML>=6.0, <7.0.0 | ||
| graphviz>=0.18.2, <0.21 |
There was a problem hiding this comment.
These aren't dev dependencies, are they? I'm not sure why we need pyYAML, but graphviz is referenced in our production package. As such, it should probably be in the metricflow-semantics dependency set, or at least listed as an optional dependency there.
There was a problem hiding this comment.
pyYAML is needed to parse YAML configuration files for tests. graphviz is used in the tests and the CLI. Let me put it back as a regular dependency until I figure out what to do with it.
pyproject.toml
Outdated
| @@ -113,11 +111,11 @@ MF_TEST_ADAPTER_TYPE="duckdb" | |||
| MF_SQL_ENGINE_URL="duckdb://" | |||
| # This allows us to use the classes in the `dbt-metricflow` package for tests without installing the package. | |||
| # `dbt-metricflow` can't be installed as it has `metricflow` as a dependency. | |||
| PYTHONPATH="./dbt-metricflow" | |||
| PYTHONPATH="metricflow-semantics:dbt-metricflow" | |||
There was a problem hiding this comment.
Ah, nice, I see, so this should maybe make the install from the last PR editable? Let's give it a try!
|
Can you clarify what you mean by |
|
I think if you try to use the CLI without a separate installation of graphviz the import will fail and metricflow won't load. |
|
Yeah, prior to the latest commit, that would have been the case. Now that |
There was a problem hiding this comment.
Nice, thanks!
Graphviz appears to have basically no dependencies so maybe we just keep it there until somebody hits a version conflict, but since they haven't had a minor version release since 2022 I don't think this is an issue.
We can also make it optional, condition the import appropriately, and then make the CLI package pull in the optional dep from metricflow-semantics, but I'm more inclined to move the relevant module and maybe rename that one graphviz_whatever property we have floating around - I looked at the logic and it's portable, the only unpleasant thing is it adds a typing dependency on the DagNode class, but when we harden the API we can replace that with some kind of MFDagNode protocol.
plypaul
force-pushed
the
p--py312--12
branch
2 times, most recently
from
73c97e8 to
6a46f13
Compare
Some of the dependencies listed for `metricflow` should instead be dependecies in `metricflow-semantics` or part of the dev environment.
Dependencies between `metricflow` and `metricflow-semantics` should largely be the same.
Description
This PR moves dependencies for
metricflowso that they are shared withmetricflow-semanticsand makes some updates to put them in the proper sections.