GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: Add integration tests for dbt import with local dbt project by Copilot · Pull Request #5877 · feast-dev/feast Conversation
|
There was an error while loading. Please reload this page.
|
@YassinNouh21 mind reviewing? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| with: | ||
| enable-cache: true | ||
|
|
||
| - name: Install dependencies |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copilot we should use the Makefile to install dependencies here
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Updated to use make install-python-dependencies-ci in commit 621cefb.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| path: ':memory:' | ||
| threads: 1 | ||
|
|
||
| bigquery_test: |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copilot we should noly test with the sqlite setup, no need for us to use bigquery or snowflake imo
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Removed BigQuery and Snowflake profiles, kept only DuckDB setup in commit e25d08f.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: franciscojavierarceo <[email protected]>
| uv pip install --system -e "." | ||
| uv pip install --system dbt-artifacts-parser pytest pytest-xdist | ||
|
|
||
| - name: Run dbt integration tests |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copillot we should also run the standard dbt commands; i.e,.
dbt run
dbt build
dbt testafter that we should then test the new feast cli and test that feast materialization works from a dbt model into a sqlite online store.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added dbt commands (deps, build, test) and Feast CLI testing with SQLite online store in commit ba38da0. The workflow now:
feast dbt import command
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: franciscojavierarceo <[email protected]>
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
See inline comments for specific fixes needed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
fix those issues copilot
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Found several API mismatches that will cause test failures
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| def test_create_feature_view(self, parser): | ||
| """Test creating Feast FeatureView from dbt model.""" | ||
| mapper = DbtToFeastMapper(data_source_type="bigquery") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
entity.join_keys should be entity.join_key - Entity uses singular string, not a list
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copilot please revise
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| # Check that schema excludes entity and timestamp columns | ||
| feature_names = {f.name for f in feature_view.schema} | ||
| assert "driver_id" not in feature_names # Entity column excluded |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
feature_view.entities[0] contains entity names as strings, not Entity objects. Should be: assert feature_view.entities[0] == entity.name
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copilot please revise
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| def test_code_generation_workflow(self, parser): | ||
| """Test workflow that generates Python code.""" | ||
| models = parser.get_models(model_names=["driver_features"]) | ||
|
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Incorrect count. With 3 models you get: 3 DataSources + 3 Entities + 3 FeatureViews = 9 objects total. Should be: assert len(all_objects) == 9
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@copilot please revise
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@franciscojavierarceo Found some issues in this PR that need attention: Critical bugs (will cause test failures):
Architecture concerns:
The inline comments on lines 191, 215, 488 show the exact fixes needed for the critical bugs. Happy to help if you need clarification on any of these. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@YassinNouh21 you have to tag |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@franciscojavierarceo I thought he will continue working on it 😂 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: franciscojavierarceo <[email protected]>
|
@copilot tests are failing |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…generate it Co-authored-by: franciscojavierarceo <[email protected]>
Fixed in commit 586f825. The issue was an incorrectly pre-committed manifest.json from unit tests (with wrong project name "sample_dbt_project" instead of "feast_integration_test"). The manifest is now generated fresh by |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Add integration tests for dbt import with local dbt project
There was an error while loading. Please reload this page.