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

Migrate from micromamba to pixi #230

Merged
merged 27 commits into from
Jun 24, 2024
Merged

Migrate from micromamba to pixi #230

merged 27 commits into from
Jun 24, 2024

Conversation

kklein
Copy link
Collaborator

@kklein kklein commented Jun 21, 2024

No description provided.

@kklein kklein added the ready label Jun 21, 2024
@kklein kklein changed the title Draft usage of pixi. Migrate from micromamba to pixi Jun 21, 2024
@@ -2,17 +2,14 @@ version: 2
build:
os: ubuntu-20.04
tools:
python: mambaforge-4.10
python: mambaforge-latest
commands:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@pavelzw pavelzw Jun 21, 2024

Choose a reason for hiding this comment

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

i took https://github.com/prefix-dev/pixi/tree/main/examples/readthedocs-override as inspiration. but maybe the readthedocs-extend is a better fit for sphinx ...

@kklein
Copy link
Collaborator Author

kklein commented Jun 21, 2024

Hi @pavelzw ! Any idea about the origin of this failure?

https://github.com/Quantco/datajudge/actions/runs/9613959735/job/26517778652?pr=230

@pavelzw
Copy link
Member

pavelzw commented Jun 21, 2024

    × unknown environment 'snowflake-sa2'

we commented this env out because it took too long to solve on windows... i think this is an issue with pixi. for now, let's deactivate it. i'll file an issue

@pavelzw
Copy link
Member

pavelzw commented Jun 21, 2024

mamba-org/resolvo#49

pixi.toml Outdated Show resolved Hide resolved
pixi.toml Outdated Show resolved Hide resolved
pixi.toml Outdated
db2-py38 = ["db2", "py38", "test"]
db2-py39 = ["db2", "py39", "test"]
db2-py310 = ["db2", "py310", "test"]
# db2-py311 = ["db2", "py311", "test"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you have some environments commented out.

Copy link
Member

Choose a reason for hiding this comment

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

these are not solvable but might be in the future. no strong opinions on whether we should keep them commented out or remove the lines

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't care if they're commented. I just wondered because this is the reason a PR merging into this branch is failing and wanted to understand why. I'd say leave them commented out for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the comment for now

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
- latest
- 11
env:
- postgres-py38
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look complete. We should have the cross product of python, postgres and sqlalchemy versions. Here it looks like python and sqlalchemy are decoupled now.

This observation applies to the others too.

Copy link
Member

Choose a reason for hiding this comment

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

i think testing against the border of the cross product matrix should be fine as well, otherwise this can easily explode. i also don't think we would catch significantly more errors that way 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're aware it can explode. That's why in the obscure databases we covered a few combinations.

We can discuss how we effectively do integration tests. Doing the borders of the testing phase space is fine by me.

- db2-py311
- db2-py312
- db2-sa1
- db2-sa2
Copy link
Collaborator

Choose a reason for hiding this comment

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

We werent testing db2 with sqlalchemy 2 before.

- snowflake-py311
- snowflake-py312
- snowflake-sa1
- snowflake-sa2
Copy link
Collaborator

Choose a reason for hiding this comment

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

We werent testing sqlalchemy 2. Although It might be interesting to give it a second try.

Furthermore, we weren't testing all python versions for these more "esoteric" databases to reduce time, and costs.

always_run: true
require_serial: true
pass_filenames: false
- id: black
Copy link
Collaborator

@ivergara ivergara Jun 21, 2024

Choose a reason for hiding this comment

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

The target versions for black and pyupgrade are gone from here. Are they implicitly using the value for requires-python in pyproject.toml?

Copy link
Member

Choose a reason for hiding this comment

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

probably. but we want to move to ruff anyway in a separate PR (which definitely uses requires-python)

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.14%. Comparing base (c52ad81) to head (2d4714f).
Report is 39 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
- Coverage   92.49%   92.14%   -0.36%     
==========================================
  Files          18       18              
  Lines        1973     1973              
==========================================
- Hits         1825     1818       -7     
- Misses        148      155       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kklein
Copy link
Collaborator Author

kklein commented Jun 23, 2024

@ivergara any idea why the BigQuery authentication is failing?

@ivergara
Copy link
Collaborator

@ivergara any idea why the BigQuery authentication is failing?

Not really. Nothing seems to have changed in this PR. My suggestion for now is to skip this integration test and deal with it separately.

@kklein kklein marked this pull request as ready for review June 24, 2024 08:02
@kklein kklein requested review from ivergara and pavelzw June 24, 2024 08:02
@kklein
Copy link
Collaborator Author

kklein commented Jun 24, 2024

@pavelzw I compared the readthedocs pages manually; the version number/PR number is the only difference I came across

@kklein
Copy link
Collaborator Author

kklein commented Jun 24, 2024

Thanks for your help @ivergara and @pavelzw :)

@kklein kklein merged commit 73c1792 into main Jun 24, 2024
57 of 58 checks passed
@kklein kklein deleted the migrate_to_pixi branch June 24, 2024 12:35
kklein added a commit that referenced this pull request Jun 25, 2024
* Draft usage of pixi.

* Adapt more integration tests.

* Adapt bigquery integration test task.

* Migrate docs deployment to pixi.

* Update build and publish.

* Run postinstall for readthedocs.

* Adapt liniting to rely on pixi.

* Add pytest as dependency.

* Adapt readthedocs configuration.

* Update development instructions.

* Switch to pixi in helper script.

* Adapt development.rst.

* Comment out impala run for debugging.

* Move flit to host-dependencies.

* Remove redundant dependency.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Add color to ci tests.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Remove configurations which were erroneously added.

* Remove configurations which were erroneously added.

* Consistently use ubuntu-latest.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Also run unit tests on macos and windows.

* Fix impala test syntax.

* Downgrade sqlalchemy version for impala tests.

* Disable BigQuery tests for now.

---------

Co-authored-by: Pavel Zwerschke <[email protected]>
kklein added a commit that referenced this pull request Jun 25, 2024
* Draft usage of pixi.

* Adapt more integration tests.

* Adapt bigquery integration test task.

* Migrate docs deployment to pixi.

* Update build and publish.

* Run postinstall for readthedocs.

* Adapt liniting to rely on pixi.

* Add pytest as dependency.

* Adapt readthedocs configuration.

* Update development instructions.

* Switch to pixi in helper script.

* Adapt development.rst.

* Comment out impala run for debugging.

* Move flit to host-dependencies.

* Remove redundant dependency.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Add color to ci tests.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Remove configurations which were erroneously added.

* Remove configurations which were erroneously added.

* Consistently use ubuntu-latest.

* Update .github/workflows/ci.yaml

Co-authored-by: Pavel Zwerschke <[email protected]>

* Also run unit tests on macos and windows.

* Fix impala test syntax.

* Downgrade sqlalchemy version for impala tests.

* Disable BigQuery tests for now.

---------

Co-authored-by: Pavel Zwerschke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants