-
Notifications
You must be signed in to change notification settings - Fork 5
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
Devcontainer support including Duckdb bootstrapped with minimal synthetic data #57
base: main
Are you sure you want to change the base?
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.
Nice work!! Feel free to ignore suggestions - but I think we can simplify some of the settings/extensions used here. Happy to debate
// "forwardPorts": [], | ||
// Use 'postCreateCommand' to run commands after the container is created. | ||
"postCreateCommand": "bash ./.devcontainer/scripts/postCreate.sh", | ||
"postStartCommand": "bash ./.devcontainer/scripts/postStart.sh", |
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.
This postStart script is not needed
"postStartCommand": "bash ./.devcontainer/scripts/postStart.sh", |
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.
Possibly - and this may be a relic from a fix to get devcontainers working for several projects on my machine (Windows with Docker Desktop using WSL2). If someone can confirm that git does not throw permission errors without that line, please feel free to remove it. It is borrowed from another devcontainer that does a bunch of stuff after starting.
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.
The postStart.sh is needed in order to install the duckdb pieces.
I'm working off that shell script so that these happen after the Setup dbt section
I haven't added fancy logic to detect which OS.
My assumption is that for Github Codespaces that it is good enough for demoing for the symposium.
A longer term solution will be needed.
`
Setup dbt
echo "Setting up duckdb synthetic data directory"
mkdir ./data
echo "Installing Duckdb in Github Codespaces which is Ubuntu based OS"
wget https://github.com/duckdb/duckdb/releases/download/v1.0.0/duckdb_cli-linux-amd64.zip
unzip duckdb_cli-linux-amd64.zip
echo "Initialize the duckdb file and exit duckdb"
/workspaces/dbt-synthea/duckdb ./data/synthea_omop_etl.duckdb -s .quit
echo "Debugging dbt to check the connection to duckdb"
dbt debug
echo "Configure dbt dependent Python packages"
dbt deps
echo "Seed dbt duckdb with data"
dbt seed
echo "Compile and Build the dbt project"
dbt compile
dbt build
`
"dbt.dbtPythonPathOverride": "./dbt-env/bin/python3" | ||
} | ||
} | ||
}, |
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.
Valid json
}, | |
} |
dbt-core==1.8.2 | ||
dbt-duckdb==1.8.1 | ||
pre-commit==3.7.1 | ||
black | ||
python-dotenv |
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 think we don't need a requirements.txt file for this (if we only need to install dbt-duckdb
)
dbt-core==1.8.2 | |
dbt-duckdb==1.8.1 | |
pre-commit==3.7.1 | |
black | |
python-dotenv |
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 am unsure of the purpose of the root requirements.txt
too? but I think thats for a different PR!
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.
A devcontainer is for a consistent development experience - and black
and pre-commit
are the bare minimum and ensure that code contributions meet a minimum standard. We should have sqlfluff
included explicitly in there too - I think it gets installed anyway as part of dbt.
I can see that the pre-commit -config.yaml
file is very minimal - and can be improved as well. Possibly a subset of what is here -> https://github.com/vvcb/dbt-synthea/blob/vc/databricks/.pre-commit-config.yaml
Typically, one wouldn't need a separate requirements file within the devcontainer and the project requirements.txt
should be sufficient. The new PR could address this with a single requirements.txt file for the project with database-specific dependencies and dependencies required only for development (such as black, pre-commit, etc.) specified as optional dependencies. This should allow the user to install the project with pip install dbt_synthea[postgres,dev]
for instance.
Development dependencies are going to be different from dependencies required for just running the code.
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.
Thank you for your explanation.
Ah that makes more sense, I trimmed out some of it as it wasn't referenced anywhere else and didn't see why it would be needed ~ it's probably fine to leave it in for now and can have another PR to tighten up precommit actions
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.
can/should this be updated now that #67 is merged in?
# Set up git | ||
git config --global --add safe.directory /workspaces/dbt-synthea | ||
git config --global init.defaultBranch main | ||
|
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.
Happy to be corrected, but I think some of this is unnecessary - but maybe is required to defend against WSL2 instances (as I know git can get funny with Windows' ACLs)? Can leave in but I think not needed!
Regardless, the default branch doesn't need to be changed as the project already exists
# Set up git | |
git config --global --add safe.directory /workspaces/dbt-synthea | |
git config --global init.defaultBranch main | |
# Set up git | |
git config --global --add safe.directory /workspaces/dbt-synthea | |
git config --global init.defaultBranch main | |
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.
Agree regarding default branch. But, the add safe.directory directive is required on when running this on WSL2.
git config --global --add safe.directory /workspaces/dbt-synthea | ||
|
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 needs to be run on every postStart (if it has already been done onCreate), if at all
git config --global --add safe.directory /workspaces/dbt-synthea |
"python.terminal.activateEnvInCurrentTerminal": true, | ||
"python.defaultInterpreterPath": "./dbt-env/bin/python3", | ||
"dbt.dbtPythonPathOverride": "./dbt-env/bin/python3", |
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.
"python.terminal.activateEnvInCurrentTerminal": true, | |
"python.defaultInterpreterPath": "./dbt-env/bin/python3", | |
"dbt.dbtPythonPathOverride": "./dbt-env/bin/python3", |
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.
Hah 😄 I should have scrolled here before responding to the previous comment. Agree with this change - the user should be responsible for managing the environment rather than making this part of the codebase. But, this needs to be discussed with the others as it will change the behaviour of vscode for users.
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.
so, i had set it up this way so that folks using VS Code would get their virtualenv activated each time they open the repo, without having to remember to activate it. if there's a way to instruct people to set this up without committing it to the repo, i'm open to doing that instead - is there?
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.
Setting up and activating an environment should be the user's responsibility and outside of source control. What if someone wants to use uv
or conda
to manage their environments or use something other than vscode for development?
I would propose moving this to the documentation in README and modifying my devcontainer hack to get rid of setting up a virtual environment within a devcontainer - which as @lawrenceadams rightly pointed out, is redundant.
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.
In this case should we add .vscode to .gitignore? (and then maybe we could add this settings json file to an extras folder so users can easily copy it to their local if they want to use it without thinking too much about it)
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.
My preference is to keep .vscode under source control to share settings (eg. spaces Vs tabs, task configuration, etc.) - there is a good discussion here - https://stackoverflow.com/questions/32964920/should-i-commit-the-vscode-folder-to-source-control
However, environment activation steps can simply go in README rather than in code. I would expect the user to be able to setup a virtual environment if they are planning to use DuckDb, Postgres and dbt. If not, good time to learn anyway.
I will update this PR (with rebase).
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.
Sounds good to me! Please go ahead and include that change in this PR :)
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.
Forgot to bump this! Nice catch
"extensions": [ | ||
"davidanson.vscode-markdownlint", | ||
"editorconfig.editorconfig", | ||
"mads-hartmann.bash-ide-vscode", | ||
"mechatroner.rainbow-csv", | ||
"ms-python.black-formatter", | ||
"ms-python.python", | ||
"ms-python.vscode-pylance", | ||
"njpwerner.autodocstring", | ||
"innoverio.vscode-dbt-power-user" | ||
], |
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.
Some of these are unused
"extensions": [ | |
"davidanson.vscode-markdownlint", | |
"editorconfig.editorconfig", | |
"mads-hartmann.bash-ide-vscode", | |
"mechatroner.rainbow-csv", | |
"ms-python.black-formatter", | |
"ms-python.python", | |
"ms-python.vscode-pylance", | |
"njpwerner.autodocstring", | |
"innoverio.vscode-dbt-power-user" | |
], | |
"extensions": [ | |
"davidanson.vscode-markdownlint", | |
"mechatroner.rainbow-csv", | |
"ms-python.python", | |
"ms-python.vscode-pylance", | |
"njpwerner.autodocstring", | |
"innoverio.vscode-dbt-power-user" | |
], |
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 you sure you want to get rid of black?
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 forgot there were 3 python files!
"python.defaultInterpreterPath": "./dbt-env/bin/python3", | ||
"dbt.dbtPythonPathOverride": "./dbt-env/bin/python3" |
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.
can trim down on unnecessary venv configs
"python.defaultInterpreterPath": "./dbt-env/bin/python3", | |
"dbt.dbtPythonPathOverride": "./dbt-env/bin/python3" |
@lawrenceadams , thank you for reviewing this. Agree with most things - but this was to get the devcontainer working with the existing codebase in preparation for the OHDSI symposium software demo to allow users to spin this up in their own GitHub codespaces. Disagree with a few. Very happy for any of this to be modified as long as it works for everyone. |
In all honesty is good to go for all intents and purposes, a lot of these issues are a reflection of python being annoying to manage packages with IMHO! |
@vvcb thanks for working on this - i left a couple comments. also needs a rebase to incorporate recent changes 😄 i had been waiting for Heidi to take a look since Codespaces was her idea ... will ping in Teams to make sure she saw this. |
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.
Please add the additions to postCreate.sh so that the duckdb install and the initialization of the database will allow the dbt commands to run properly.
// "forwardPorts": [], | ||
// Use 'postCreateCommand' to run commands after the container is created. | ||
"postCreateCommand": "bash ./.devcontainer/scripts/postCreate.sh", | ||
"postStartCommand": "bash ./.devcontainer/scripts/postStart.sh", |
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.
The postStart.sh is needed in order to install the duckdb pieces.
I'm working off that shell script so that these happen after the Setup dbt section
I haven't added fancy logic to detect which OS.
My assumption is that for Github Codespaces that it is good enough for demoing for the symposium.
A longer term solution will be needed.
`
Setup dbt
echo "Setting up duckdb synthetic data directory"
mkdir ./data
echo "Installing Duckdb in Github Codespaces which is Ubuntu based OS"
wget https://github.com/duckdb/duckdb/releases/download/v1.0.0/duckdb_cli-linux-amd64.zip
unzip duckdb_cli-linux-amd64.zip
echo "Initialize the duckdb file and exit duckdb"
/workspaces/dbt-synthea/duckdb ./data/synthea_omop_etl.duckdb -s .quit
echo "Debugging dbt to check the connection to duckdb"
dbt debug
echo "Configure dbt dependent Python packages"
dbt deps
echo "Seed dbt duckdb with data"
dbt seed
echo "Compile and Build the dbt project"
dbt compile
dbt build
`
As discussed at the dbt sync community call on 2024-09-20, this PR adds devcontainer support to the project.
Key features:
./dbt-env/bin/python3
to allow the extension to work - this requires a longer-term fix where the extension recognises this automatically.The user will have to run
dbt run
to create the rest of the models.