Update requirements files, use pyproject.toml, update readme.

[wylie102]

This commit adds a pyproject.toml for managing dependencies and dev tool configurations.

Dev dependancies are updated slightly:

• Ruff replaces black, but is configured to format in almost exactly the same way.
• The script dependencies such as beautifulsoup are added to the dev group.
• Codespell is added as a tool, this catches common spelling mistakes using a dictionary of common errors (so it won't highlight every little issue in function/variable names etc). There is a list of ignored words in it's config in the pyproject.toml file, feel free to add to it. Run it using condespell <path/to/files>.

The requirements files are updated to specify the current versions of duckdb and the dbt connectors, for those wanting to stick with pip.
Also the common.in is renamed to dev.in and we have a dev.txt file. The duckdb.txt and postgres.txt no longer include the dev dependencies.

The readme file is updated to show uv vs pip instructions.

[wylie102]

Pyproject.toml file.

Install duckdb dev dependencies with uv sync --group duckdb --dev

Also try out the new codespell tool that looks for common spelling mistakes. Use it with codespell <path/to/files>. I ran it on the models and it picked up a few issues from the yml files, so it seems to work. We could maybe add it as a hook/test to pick up any mistakes.

There is also BasedPyright which has it's own vs code extension, I think vs code will detect it and ask if you want to temporarily turn off mypy if using it. We can remove either of these if they aren't useful or if others are preferred.

[katy-sadowski]

Looking good! What are your thoughts on actually ditching the pip instructions altogether and only supporting uv? I think it's a bit confusing to have multiple ways to do the same thing. If we provide a bit more guidance on the uv installation, I think it should be OK, unless there's some downside you know of?

Edit: Forgot to mention, the setup instructions with uv work for me on a fresh clone of the repo :)

[katy-sadowski]

this is not working for me anymore. i thought it might be because this line was referencing dbt-env from the old instructions:

dbt-synthea/.vscode/settings.json

Line 7 in 8fd7e0e

"python.defaultInterpreterPath": "./dbt-env/bin/python3",

however, even after updating to .venv it's still not working. any ideas?

(ps, i realize the inclusion of this VS code stuff is a bit controversial but i really would like to make this bit seamless for folks less familiar with environment setup)

[wylie102]

I'm not sure, you can do uv venv <name> and then it will create a named directory. So uv venv dbt-env should create it in the same place as the pip instructions. I didn't use that in the instructions because by default uv creates a .venv folder (with sync/run/init) commands so those already using it would be used to that. Is this the part that started working after a fresh install?

There is sometimes some slight strangeness around vs code picking up the correct python interpreter with uv, but it only seems to be for a small number of people and different things seem to fix it for different people. Here is the github issue, they also recommend this plugin as a possible fix. I have my terminal set up to work similarly to this, when I cd into a directory it will look for and activate the closest environment folder (heirachically).

I'm not sure why that issue is on the uv github and not vs code since it seems like a vs code problem, and the astral people don't seem to be able to recreate the issue. I've never really had an issue with setting the python interpreter to use .venv/bin/python or .venv/bin/python3.13 and it seems to change that automatically when switching projects, although it's possible that's something I set up a while ago and then forgot about.

One area that I was getting an issue was with the sqlfluff vscode extension complaining about the dbt-templater not being available, even though it is installed in the environment. Although again that seemed to be an issue with the extension. How do you have it set up and are you getting any warnings like that? I just want to make sure that isn't an issue with the local environment.

[katy-sadowski]

OK wow, the hack suggested in the description of astral-sh/uv#9637 works! I updated the default interpreter in .vscode/settings.json as follows:

"python.defaultInterpreterPath": "./.venv",

The first time you are in VS Code, you either need to enter the full path manually OR close (not reload) the window and reopen. Then subsequent times, the env activates automatically.

[wylie102]

OK wow

Yeah it's quite a lot isn't it. It seems like at least one of the fixes works for most people though.

the hack suggested in the description of astral-sh/uv#9637 works! I updated the default interpreter in .vscode/settings.json as follows:

"python.defaultInterpreterPath": "./.venv",

When I did this I found that using ./.venv/bin worked for me but not ./.venv. Could you check and see if that works for you? Because i'd be tempted to include that. The plugin they mention also worked, I think it's called Python Envy.

[katy-sadowski]

Sorry I missed this!!! It works for me with ./.venv/bin as well (and out-of-the-box with the updated file in .vscode).

I might just add instructions for (1) activating the venv for the first time, and (2) subsequent times (i.e., close VS Code and reopen - this is the only way I've gotten the auto-activate to work. Reloading the window does not work). Btw, pretty sure these same instructions would apply to the repo in its current state, they were just missing before :)

Thanks!

[katy-sadowski]

is it possible to change this so we can install dependencies for multiple databases? i would like to have both duckdb and postgres requirements installed.

[wylie102]

Yeah I think it would just be uv sync --dev --group duckdb --group postgres or just uv sync --all-groups

You can do as many groups as you like it's just that they aren't additive, any sync statement results in the local environment containing only what was specified in that sync. So you have to specify everything within the same statement.

Do you want me to add those to the readme?

[katy-sadowski]

Yay that worked! Yes please, maybe another note below the first one :) thanks!

[wylie102]

Yep I'll add these and take out the pip and the requirements folder. That means updating the gh workflows file, which I've done but I don't have my fork set up to run that on push yet. I'll get that and the readme sorted tomorrow hopefully. Also see my comment about the .venv documentation

[wylie102]

What are your thoughts on actually ditching the pip instructions altogether and only supporting uv?

I don't mind it, I consider the pip instructions kind of legacy. Especially for projects like this where you don't want to install the project itself.

I think it's a bit confusing to have multiple ways to do the same thing.

This is kind of why I relegated the entire pip section to the bottom of the readme. But I can understand it's a bit cluttered and repetative.

Unless there's some downside you know of?

I was wondering if someone might be working on a device where they can't install uv for some reason. Although then I guess pipx install uv or pip install uv would still work.

It's up to you, I started using uv very shortly after starting to learn python so I've barely used pip. But there might be some who are more tied to it? I'm fine to drop it entirely though, it would let us get rid of the requirements files and just have everything work from the pyproject.toml.

Or we could move the pip instructions to a separate .md file and put that in with the requirements? I'm pretty sure we can get the requirements to be refreshed automatically by uv using pre-comit, although we'd have to remember to update the .md on a change. I'm sure there must be some tools for that too though, even just a regex would do it to turn uv run into python3.

But probably dropping it entirely is cleaner. We'd just have to update some of the github actions to use uv. But that shouldn't be an issue.

Edit: If you get a chance, take a look at basedpyright and codespell and let me know what you think. If you roll the python scripts back to before I added the type checking and add the basedpyright extension in vs code it should give you an idea of what it looks like, and then try commenting/uncommenting some of the rules in the pyproject.toml. They have very good docs on their site

[lawrenceadams]

Its probably dropping it entirely, but the reason I never advised / brought in uv - is precisely as its quite an opinionated way of doing it, and makes it harder for new people to use it (even if uv is objectively better)

[wylie102]

Its probably dropping it entirely, but the reason I never advised / brought in uv - is precisely as its quite an opinionated way of doing it, and makes it harder for new people to use it (even if uv is objectively better)

Yeah, this is a much more succinct way of putting it 😂.

And practically, if the user is very new to everything and are just copy pasting the commands, then asking them to run curl -LsSf https://astral.sh/uv/install.sh | sh or pip install uv before the other commands will make no less sense to them and not be any more difficult.

And if they're some curmudgeonly old dev who refuses to move on from pip then they probably know enough about pip to just copy paste the groups from the .toml into a their own requirements.in and go from there.

So it probably won't cause any issues with anyone just dropping it entirely.

[katy-sadowski]

And practically, if the user is very new to everything and are just copy pasting the commands, then asking them to run curl -LsSf https://astral.sh/uv/install.sh | sh or pip install uv before the other commands will make no less sense to them and not be any more difficult.
And if they're some curmudgeonly old dev who refuses to move on from pip then they probably know enough about pip to just copy paste the groups from the .toml into a their own requirements.in and go from there.
So it probably won't cause any issues with anyone just dropping it entirely.

This is my thinking as well. Let's go full uv! @wylie102 if you don't mind adding that uv install step as a note, that'd be great!

[wylie102]

@lawrenceadams would you mind taking a look at the change i've made to the github actions? I just changed it to install uv and get uv to use the pyproject.toml, which seems pretty straightforward but I haven't used gh actions much outside of setting up an issues template so a second eye would be much appreciated. I'll try and set my fork up so that it runs it as well to double check it works. I assume that would be a specific step in any forked repo and wouldn't just happen by having the generate-docs.yml in place? Do you thin it's worth including that step in the dev instructions?

[wylie102]

@katy-sadowski, I was wondering about including a small section on how to get the vocabulary files. That is the part that to me was the least obvious to me. I alredy knew how todownload and run synthea, but Atlas requires:

1. Knowing about it.
2. Setting up an account.
3. Running that cpt4 thing after downloading them, which isn't very obvious and I think might have been the cause of the vocab related errors that that cedar prince chap mentioned in the big data discussion (here).

So I thought possible a section on how to do these steps?

[wylie102]

Okay after a bit of trial and error I have added a test workflow. Unlike the docs one this should run on any push, so if you push on any branch.

So if you push a change to your own branch it should run - dbt build, dbt test, and dbt docs generate - using uv and the duckdb requirements.

I've updated the generate docs to use the same methods so it should run (although it hasn't been triggered in my fork as I haven't pushed these to main.

I've also updated the readme.

The results of the tests aren't currently persisted, possibly we could add another part to run this on a pull request and include the results in it in a later PR. We could also add a run of the formatting tools etc if we wanted. Or possibly the recce tool that lawrence mentioned (or a ducklake snapshot if we are feeling adventurous) But it's probably best just to check that this works correctly in a simple version first.

Take a look and let me know if there are any changes to that or to the readme that need to be made.

[lawrenceadams]

Mostly pretty good - as per comments if those make sense

[lawrenceadams]

I'd get rid of workflow dispatch and change it to run on PR instead of branches, otherwise you're gonna needlessly burn through GH compute credits

[lawrenceadams]

on:
  pull_request:
    branches: [main]
    paths:
      - models/**
      - seeds/**
      - snapshots/**
      - tests/**
      - macros/**
      - dbt_project.yml
      - packages.yml
      - requirements.txt
    types:
      - opened
      - synchronize
      - reopened

This is what I tend to go for - only runs when there's a PR open for something

Unsure but @katy-sadowski may need to change a setting so that only maintainers can authorise workflow runs

[wylie102]

Oh I literally didn't know about the gh compute credits, yeah PR makes more sense. That's also why I hadn't removed the documents build, I didn't think there was really a down side to checking it worked (even though I couldn't really think of a case that it wouldn't if run and test did). But I'll change both of those.

I mostly was setting this up to run on a contributors fork, would that then come from their credits quota then? Do you k ow if we move it to or does it charge the ohdsi repository or the individual forks?

[wylie102]

How about keeping workflow dispatch so someone can manually trigger the tests if they want to check them before opening a PR but only let it be used in forks?

So something like:

name: Test dbt duckdb

on:
  pull_request:
    branches:
      - main
    paths:
      - macros/**
      - models/**
      - scripts/**
      - seeds/**
      - snapshots/**
      - tests/**
      - dbt_project.yml
      - .github/workflows/**
  workflow_dispatch:

jobs:
  test-dbt:
    if: |
      (github.event_name == 'pull_request' && github.repository == 'ohdsi/dbt-synthea') ||
      (github.event_name == 'workflow_dispatch' && github.repository != 'ohdsi/dbt-synthea')
    runs-on: ubuntu-latest

Also, what is the reason to not include changes to github workflows? I'd have thought if we're changing the workflow we'd want to run the test, especially as you can't run it locally? Or do you just make the changes and then open a dummy pr to test?

[katy-sadowski]

The proposal to run actions on PRs to main sounds good to me! And I've never used/set up workflow dispatch before, but I think it's OK to have it as an option in the repo. It should only be available to people with write access to the repo.

GH Actions runs in forks will be charged to the user who owns the fork, and need to be set up in the fork before they'll run automatically.

[lawrenceadams]

@wylie102 It's okay to have for testing your workflows but sort of redundant once they're setup (these workflows dont have side effects)

Yeah that's valid - the workflow I borrowed that from wasn't very complex and so I was fairly confident the dbt commands it would run

[lawrenceadams]

Are these all needed explicitly rather than as a known dependency? Why is jinja explicitly mentioned for example if it's a known dependency of dbt (for example)

[wylie102]

I just copied them across from common.in and added the script dependencies.

dbt-synthea/requirements/common.in

Lines 1 to 5 in e119df6

	pre-commit==3.8
	black==24.8
	sqlfluff==3.2
	sqlfluff-templater-dbt==3.2
	Jinja2>=3.1.6

If it's not being used independently from dbt (like with a linter/formatter extension or something) I'll take it out.

[katy-sadowski]

I added Jinja2 because there was a Dependabot alert for the package and this was the recommended fix (#122).

[wylie102]

Looking at the dbt requirements, they specify >=3.13, so it might be worth us still pinning it to >=3.15. It won't do any harm. I'll probably move it to the duckdb and Postgres groups though, as I haven't specified the version on any of the dev dependencies as I thought we could figure it out ourselves if there were any issues, and if we run into any big differences between the versions we can always add a version specification later.

[lawrenceadams]

Why do this here?

[wylie102]

Mostly just a hangover from me using your generate docs as a base, didn't see the harm in keeping it but I wasn't thinking about the compute credits. I'll bin it since it doesn't add anything anyway.

[lawrenceadams]

Ah yeah its more as its redundant in this instance as there is the main pipeline run

[katy-sadowski]

@katy-sadowski, I was wondering about including a small section on how to get the vocabulary files. That is the part that to me was the least obvious to me.

@wylie102 I totally thought I replied to this a while back but I must have forgotten to submit my comment. The instructions for downloading the vocabulary are here: https://github.com/OHDSI/Vocabulary-v5.0/wiki/General-Structure,-Download-and-Use. Do you think linking to this in the README would suffice?