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

airbyte ci test to support --extras #36527

Merged
merged 2 commits into from
Mar 27, 2024
Merged

Conversation

erohmensing
Copy link
Contributor

@erohmensing erohmensing commented Mar 26, 2024

Our airbyte-ci test command supports groups but not extras. This PR allows configuring extras to install if necessary (like when running the unit tests in the upstack PR), and renames to better distinguish between the groups.

We probably don't need to include dev as an optional group since it's implicitly installed, but I didn't mess with that here, for one because I wanted to test the rename.

For usage see upstack PR.

Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Mar 27, 2024 4:45am

Copy link
Contributor Author

erohmensing commented Mar 26, 2024

@@ -30,6 +30,7 @@ jobs:
with:
# Note: expressions within a filter are OR'ed
filters: |
# This list is duplicated in `pipelines/airbyte_ci/test/__init__.py`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a note for me when I was working on the upstack PR

There is some weird stuff around keepign this list up to date and also the output of changes being calculated differently from the --modified flag that we should look into later but I haven't addressed in this stack.

@erohmensing erohmensing marked this pull request as ready for review March 27, 2024 04:43
@erohmensing erohmensing requested a review from a team as a code owner March 27, 2024 04:43
Base automatically changed from ella/lock to master March 27, 2024 04:44
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Nit stacking remark: I would have first renamed extra_poetry_groups to optional_poetry_groups in a PR and would have introduced the poetry_extras in a different one.

  • What's exactly the difference between groups and extras?
  • Is there a way to install everything in a single poetry command instead of having packages to specify the same things?

@erohmensing
Copy link
Contributor Author

erohmensing commented Mar 27, 2024

@alafanechere

What's exactly the difference between groups and extras?

They are admittedly confusing. Groups are only used to organize dependencies, and the very imporant thing is Dependency groups, other than the implicit main group, must only contain dependencies you need in your development process. Installing them is only possible by using Poetry., i.e. they can only be dev dependencies. I think they're basically like dev-extras. I have no idea why you can't organize production dependencies - seems a bit ridiculous.

To declare a set of dependencies, which add additional functionality to the project during runtime, use [extras](https://python-poetry.org/docs/pyproject/#extras) instead. Extras can be installed by the end user using pip., i.e. they are similar to setup.py extras in that they can contain code that is shipped with the package and can be installed by specifying extras.

Is there a way to install everything in a single poetry command instead of having packages to specify the same things?

Not that I can see. There is no command to install all groups, and installing groups is fundamentally different from installing extras.

I guess I don't know if we really need to support groups at all, since we only use dev groups, and that one is implicitly always downloaded (e.g. poetry install == poetry install --with dev.

@alafanechere
Copy link
Contributor

I guess I don't know if we really need to support groups at all, since we only use dev groups, and that one is implicitly always downloaded (e.g. poetry install == poetry install --with dev.

You are right. I think I originally eagerly implemented this because I did not know that poetry install installed dev too and originally packages had different extras name in setup.py for dev deps: tests / dev etc.

Thanks for the clarification on group vs extras. I think it makes sense from a DX standpoint. You don't have to care too much when you run poetry install manually. And when you install packages at build time you can use a flag to ignore dev deps.

Copy link
Contributor Author

erohmensing commented Mar 27, 2024

Merge activity

@erohmensing erohmensing merged commit 1ca0e74 into master Mar 27, 2024
43 checks passed
@erohmensing erohmensing deleted the ella/airbyte-ci-test-extras branch March 27, 2024 18:16
nurikk pushed a commit to nurikk/airbyte that referenced this pull request Apr 4, 2024
Our `airbyte-ci test` command supports [groups](https://python-poetry.org/docs/pyproject/#dependencies-and-dependency-groups) but not [extras](https://python-poetry.org/docs/pyproject/#extras). This PR allows configuring extras to install if necessary (like when running the unit tests in the [upstack PR](https://github.com/airbytehq/airbyte/actions/runs/8446603559/job/23135651978?pr=36497)), and renames to better distinguish between the groups. 

We probably don't need to include `dev` as an optional group since it's implicitly installed, but I didn't mess with that here, for one because I wanted to test the rename.

For usage see [upstack PR.](https://github.com/airbytehq/airbyte/pull/36497/files#diff-1734d8b375bd2fc6fd8419f94ca88aad97eefaf2e78fa548c7379b28fca6456dR97-R101)
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.

3 participants