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

[#21] fix evidence can't locate local duckdb files #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxestorr
Copy link
Contributor

@maxestorr maxestorr commented Apr 28, 2024

export case sensitive evidence environment variables.

also provided instructions in .env.example on how to configure project for local use.

removed dagster_hybrid_demo from schema name that evidence sources SELECT from as the tables created by duckdb and dbt exist in the main schema, was causing error where evidence couldn't find the data sources.

export case sensitive evidence environment variables.

also provided instructions in .env.example on how to configure project
for local use.

removed `dagster_hybrid_demo.` from schema name in FROM statement as the
tables created by duckdb exist in the `main` schema, was causing error
where evidence couldn't find the data sources.
@cmpadden
Copy link
Collaborator

removed dagster_hybrid_demo. from schema name in FROM statement as the tables created by duckdb exist in the main schema, was causing error where evidence couldn't find the data sources.

I'll have to double check if this still works with MotherDuck with this removal.

@maxestorr
Copy link
Contributor Author

I'll have to double check if this still works with MotherDuck with this removal.

I'm testing it myself right now as well since it's within MotherDuck's 10GB free trial.

@maxestorr
Copy link
Contributor Author

Oh I can see it more than likely won't work, because of the dagster_hybrid_demo? in the motherduck environment variable, I'm not too familiar with motherduck I'm assuming that's a "database name" or a namespace of some sort to keep your motherduck dbs separate in the cloud. I'm probably tweaking the project for my particular usecase here so feel free to just tell me it's not a desired change, otherwise I can have a think if there's a way to configure the project so it works truly seamlessly between cloud and local compute, as I figured that was the point of the demo.

@maxestorr
Copy link
Contributor Author

maxestorr commented Apr 28, 2024

Amazingly it did work! I think because the connection string specifies the database name you don't have to in the select statement, woohoo!

image

image

@maxestorr
Copy link
Contributor Author

evidence tripped up at the last hurdle 😭 will look into this further

image

@cmpadden
Copy link
Collaborator

@maxestorr when using the evidence environment variables, did you remove the sources/dagster_hybrid_demo/connection.yaml file? I did that, and get the following error when trying to pull in sources:

$ npm run sources

> [email protected] sources
> evidence sources

[!] /Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/sources/dagster_hybrid_demo is not a valid source; skipping
ENOENT: no such file or directory, open '/Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/sources/dagster_hybrid_demo/connection.yaml'
(node:13525) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

@maxestorr
Copy link
Contributor Author

@cmpadden yes my mistake, I wanted to check that dagster was properly exporting the environment variables rather than evidence pulling the environment variable from its own connections.yaml file. I should've reverted that before committing apologies, I'll fix it now.

@cmpadden
Copy link
Collaborator

No worries - I can't seem to get it to work with env vars or the connection yaml file while connecting to MotherDuck from this branch, getting the following errors:

$ npm run sources

> [email protected] sources
> evidence sources

(node:13979) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Processing dagster_hybrid_demo
  all_ducks ✖ Catalog Error: Table with name all_birds does not exist!
Did you mean "dagster_hybrid_demo.all_birds"?
LINE 20: from main.all_birds
              ^
  top_ducks_by_region ✖ Catalog Error: Table with name top_ducks_by_region does not exist!
Did you mean "dagster_hybrid_demo.top_ducks_by_region"?
LINE 8: from main.top_ducks_by_region
             ^
  top_ducks_by_state ✖ Catalog Error: Table with name top_ducks_by_state does not exist!
Did you mean "dagster_hybrid_demo.top_ducks_by_state"?
LINE 7: from main.top_ducks_by_state
             ^
  top_ducks_by_year ✖ Catalog Error: Table with name top_ducks_by_year does not exist!
Did you mean "dagster_hybrid_demo.top_ducks_by_year"?
LINE 9: from main.top_ducks_by_year

@maxestorr
Copy link
Contributor Author

maxestorr commented Apr 28, 2024

Ah that's annoying, specifying dagster_hybrid_demo.main. in the evidence sources files results in the same error if running locally. Nevermind, I'm going to have to log off for now but I will think about a better way to implement this change.

Because the database name is specified in the motherduck connection string as md:dagster_hybrid_demo?motherduck_token=... , maybe there's some way to do something similar when specifying a local duckdb file connection string, that way whether running locally or on cloud the evidence source files can select from the same database.schema.table_name and it should just work. I will look into this further sometime later in the week, apologies I couldn't sort it sooner and thank you for your time :)

@cmpadden
Copy link
Collaborator

I take that back! The connections.yaml does work, I had forgotten to include the database name in the connection string.

$ npm run sources

> [email protected] sources
> evidence sources

(node:14177) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Processing dagster_hybrid_demo
  all_ducks ✔ Finished. 12782 rows
  top_ducks_by_region ✔ Finished. 30 rows
  top_ducks_by_state ✔ Finished. 891 rows
  top_ducks_by_year ✔ Finished. 1738 rows

However the build is failing:

$ npm run build

> [email protected] build
> evidence build

<truncate>
✓ built in 17.69s

node:internal/event_target:1096
  process.nextTick(() => { throw err; });
                           ^
ReferenceError [Error]: window is not defined
    at detect (/Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/node_modules/echarts/dist/echarts.js:123:54)
    at /Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/node_modules/echarts/dist/echarts.js:97:9
    at /Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/node_modules/echarts/dist/echarts.js:22:68
    at Object.<anonymous> (/Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/node_modules/echarts/dist/echarts.js:25:2)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)
    at Object..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Function._load (node:internal/modules/cjs/loader:1021:12)
    at cjsLoader (node:internal/modules/esm/translators:366:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:315:7)

Node.js v21.7.3
Build failed

file:///Users/colton/src/devrel-example-projects/motherduck-dagster-hybrid-compute/reports/node_modules/@evidence-dev/evidence/cli.js:178
                        throw `Build process exited with code ${code}`;
                        ^
Build process exited with code 1
(Use `node --trace-uncaught ...` to show where the exception was thrown)

@cmpadden
Copy link
Collaborator

Ah that's annoying, specifying dagster_hybrid_demo.main. in the evidence sources files results in the same error if running locally. Nevermind, I'm going to have to log off for now but I will think about a better way to implement this change.

Because the database name is specified in the motherduck connection string as md:dagster_hybrid_demo?motherduck_token=... , maybe there's some way to do something similar when specifying a local duckdb file connection string. I will look into this further sometime later in the week, apologies I couldn't sort it sooner and thank you for your time :)

Sounds good - we'll get to the bottom of this. Thanks again for all of your help.

@maxestorr
Copy link
Contributor Author

However the build is failing:

This is related to #19 I think, I did commit an update to the node modules but if you want to check that issue's thread it seems even on a fresh install of the latest version of evidence npm run build is failing for me.

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.

2 participants