Move MetricFlow CLI package to dbt-metricflow #1099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MetricFlow has not allowed Pandas installs pre-1.1 since maybe ever, so keeping a packaging.version parse check and special casing for CLI output doesn't seem worth the effort. Note the `packaging` module was only available via a transitive dependency, so this could have broken at any time.
As we move towards making MetricFlow a stand-alone library for building queries and rendering SQL, it makes sense to remove any direct query execution components and their attendant dependencies. For dependency management and package deployment purposes, we are particularly interested in removing the dbt-core dependencies from the production MetricFlow package. The lone production component relying on dbt-core is the MetricFlow CLI. In order to expedite our dependency refinement we take the step of moving the CLI under dbt-metricflow here. The packaging update here is as minimal as possible - there are a load of problems with this layout, not least of which is we need to do some pretty janky editable install operations in all of our test environments and duplicate dependencies across the two packages just to keep dev tools working. In the long run, we should move to a more refined packaging model that allows us to execute the MetricFlow test suite using the local library with a stand-alone SqlClient built for testing AND to run the same test suite using our supported adapter packages via dbt-metricflow. Doing this requires us to: 1. Overhaul our test package and fixture setups 2. Refine MetricFlow's APIs so they can be simply invoked from whatever components in dbt-metricflow require them (e.g., the CLI) 3. Re-organize our repo packaging setup to, at a minimum, move the CLI tests under dbt-metricflow While none of this is hard, exactly, it's a lot of effort that we don't have the time to make at this time, so instead we have a janky package layout and rely on a few questionable dev environment management practices. separate the production dependencies
This was referenced Mar 22, 2024
Merged
Some lint errors cropped up in CI which could not be reproduced locally, perhaps due to platform-related differences in versioning or parsing for the associated tools. This commit seeks to resolve those discrepancies by doing the following: 1. Removing the file path restrictions in the .pre-commit-config.yaml 2. Marking dbt_metricflow as a known first party package for isort All other changes are the result of running the linter with autofix enabled and these new settings in place.
tlento
added
the
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
courtneyholcomb
approved these changes
Mar 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As we move towards making MetricFlow a stand-alone
library for building queries and rendering SQL, it makes
sense to remove any direct query execution components and
their attendant dependencies. For dependency management and package
deployment purposes, we are particularly interested in removing the
dbt-core dependencies from the production MetricFlow package.
The lone production component relying on dbt-core is the
MetricFlow CLI. In order to expedite our dependency refinement we
take the step of moving the CLI under dbt-metricflow here.
The packaging update here is as minimal as possible - there are a
load of problems with this layout, not least of which is we need to
do some pretty janky editable install operations in all of our test
environments and duplicate dependencies across the two packages just
to keep dev tools working.
In the long run, we should move to a more refined packaging model that
allows us to execute the MetricFlow test suite using the local
library with a stand-alone SqlClient built for testing AND to run the
same test suite using our supported adapter packages via dbt-metricflow.
Doing this requires us to:
components in dbt-metricflow require them (e.g., the CLI)
tests under dbt-metricflow
While none of this is hard, exactly, it's a lot of effort that we don't
have the time to make at this time, so instead we have a janky package
layout and rely on a few questionable dev environment management practices.