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

[Feature] Make psycopg2-binary a default dependency, add psycopg2 as an optional dependency #6

Closed
2 tasks
mikealfare opened this issue Feb 6, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request user docs
Milestone

Comments

@mikealfare
Copy link
Contributor

mikealfare commented Feb 6, 2024

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt-postgres functionality, rather than a Big Idea better suited to a discussion

Describe the feature

In the dbt-core repo, dbt-postgres has a conditional build dependency for psycopg2. dbt-postgres defaults to psycopg2-binary unless overridden by an environment variable. With the move to pyproject.toml, we'll need a different way to handle this.

Proposal

Default to psycopg2-binary in the primary build dependencies. Offer psycopg2 via an optional extra. pyproject.toml would look like this:

# pyproject.toml
dependencies = [
    "psycopg2-binary",
    ...
]
[project.optional-dependencies]
compile = ["psycopg2"]
...

This would offer the following installation options:

  • pip install dbt-postgres: use psycopg2-binary
  • pip install dbt-postgres[no-binary]: use psycopg2

Who will this benefit?

This provides flexibility for folks installing dbt-postgres. Some users need the binary version of psycopg2 to avoid installing other build tooling. This is ideal for development. Others need the source version of psycopg2 so that it gets compiled to their machine. This is ideal for production.

Tasks

Preview Give feedback
@mikealfare mikealfare added enhancement New feature or request refinement Needs refining prior to being ready labels Feb 6, 2024
@colin-rogers-dbt
Copy link
Contributor

cc: @dbeatty10 @jtcohen6 as they probably have some context

This would be a departure for users' current install behavior as it will require people to have OS dependencies preinstalled (i.e. a C compiler, various libs etc) so we'll need to pair this with a docs update (something we'll need to do anyways as part of decoupling)

@dbeatty10
Copy link
Contributor

My typical instinct is to preserve whatever the current behavior is. In this case, that would be keeping psycopg2-binary within the dependencies, and adding psycopg2 to the optional dependencies.

What would be the advantages of the proposed order of psycopg2 as the default and psycopg2-binary as optional?

@colin-rogers-dbt
Copy link
Contributor

having given this some thought I think I agree, ultimately for production use cases users should install psycopg2 as it's significantly more performant however for users installing locally (and for our testing environments) psycopg2-binary is the better choice as it requires the least amount of configuration and is faster to install.

@mikealfare mikealfare changed the title [Feature] Add psycopg2-binary as an optional build dependency [Feature] Make psycopg2-binary a default dependency, add psycopg2 as an optional dependency Feb 7, 2024
@mikealfare
Copy link
Contributor Author

I've updated the ticket to reflect the proposal. Default to psycopg2-binary to maintain consistency and avoid compiling psycopg2 every time; offer psycopg2 as an extra.

@mikealfare mikealfare removed the refinement Needs refining prior to being ready label Feb 7, 2024
@mikealfare mikealfare added this to the 1.8.0 milestone Feb 7, 2024
@dbeatty10
Copy link
Contributor

dbeatty10 commented Feb 8, 2024

We'll want to add some kind of guidance in the v1.8 migration guide.

We don't have the DBT_PSYCOPG2_NAME environment variable documented in that many places:

So we could probably just say something like this:

Some folks may be using the DBT_PSYCOPG2_NAME environment variable to install psycopg2 instead of psycopg2-binary, possibly like this:

DBT_PSYCOPG2_NAME=psycopg2 pip install dbt

This has been replaced with the following instead:

pip install dbt-postgres[compile]

@mikealfare
Copy link
Contributor Author

I added the user docs label. I feel like we should add that on the dbt-postgres section of docs.getdbt.com.

@mikealfare mikealfare removed their assignment Feb 8, 2024
@martynydbt
Copy link

Closing for Colin

@dbeatty10
Copy link
Contributor

Assuming #41 resolved this?

If so, will there actually be a behavior change between versions of dbt-postgres that we need to add to a migration guide (requiring user_docs)?

Or does #41 just preserve the behavior of dbt-postgres when it was in the dbt-core repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request user docs
Projects
None yet
Development

No branches or pull requests

4 participants