Skip to content

Conversation

@jernejfrank
Copy link
Contributor

@jernejfrank jernejfrank commented Jun 14, 2025

Addressing #1313 and uses uv.

I don't have access to CircleCI to see the settings there (I assume the config with jobs and worklof is only part of it, since there's no mention of OS inside), would be good to let me know if anything else needs to be configured for the github actions workflow.

Changes

  • replaced circleci config with github actions
  • minor linting and bugfixes

How I tested this

  • Need to invoke CI from main repo

Notes

  • Fail-fast is disabled for now, so that I can see all of the tests in the matrix failing
  • Not sure if we need to test on all platforms
  • Adds the option to run the tests manually through the actions tab
  • some test (mostly plugins) were running only on specific python versions and run now on all supported python versions of the plugin package:
    • Ray, Dask, PySpark 3.9-3.12
    • pre-commit 3.8-3.12 (sometimes important to spot things like needing from __future__ import annotations)
    • pandas, polars, narwhals 3.8-3.12
    • Vaex 3.8-3.10

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@jernejfrank jernejfrank requested a review from skrawcz June 14, 2025 23:13
@skrawcz
Copy link
Contributor

skrawcz commented Jun 15, 2025

I don't have access to CircleCI to see the settings there (I assume the config with jobs and worklof is only part of it, since there's no mention of OS inside), would be good to let me know if anything else needs to be configured for the github actions workflow.

@jernejfrank you can see everything in the .circleci yaml and what it references. It's similar to github workflows, but we have more flexibility on when a workflow is triggered with the github way. So shouldn't be hard to replicate.

pyproject.toml Outdated
]
dependencies = [
"numpy",
"numpy<2",
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't good since 2.0 has been out for a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to remove when debugging some other dependencies issues. Removed now.

"dlt",
# furo -- install from main for now until the next release is out:
"furo @ git+https://github.com/pradyunsg/furo@main",
"furo",
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@skrawcz
Copy link
Contributor

skrawcz commented Jun 15, 2025

The SDK tests aren't passing because spark changed things. I think we just need to update the tests...

@jernejfrank
Copy link
Contributor Author

jernejfrank commented Jun 15, 2025

I don't have access to CircleCI to see the settings there (I assume the config with jobs and worklof is only part of it, since there's no mention of OS inside), would be good to let me know if anything else needs to be configured for the github actions workflow.

@jernejfrank you can see everything in the .circleci yaml and what it references. It's similar to github workflows, but we have more flexibility on when a workflow is triggered with the github way. So shouldn't be hard to replicate.

@skrawcz Ah ok, then all is migrated! I thought there might be something more since the .circleci yaml doesn't mention anything about OS to test on, etc..

Also added this to the notes of the pr:

  • some test (mostly plugins) were running only on specific python versions and run now on all supported python versions of the plugin package:
    • Ray, Dask, PySpark 3.9-3.12
    • pre-commit 3.8-3.12 (sometimes important to spot things like needing from __future__ import annotations)
    • pandas, polars, narwhals 3.8-3.12
    • Vaex 3.8-3.10

Will be fixing broken tests, but may take a while!

@jernejfrank jernejfrank force-pushed the migrate_to_github_acitions branch from 1efc74e to ffccf31 Compare June 17, 2025 19:55
@jernejfrank
Copy link
Contributor Author

Right, so SDK was passing but now the CI dissapeared. Any thoughts @skrawcz?

I am also thinking of limiting it to Linux (for now) since I saw that windows has some issues with package installation when using uv since it does a better job at finding problems than pip.

@skrawcz
Copy link
Contributor

skrawcz commented Jun 18, 2025

Right, so SDK was passing but now the CI dissapeared. Any thoughts @skrawcz?

Can you rebase against main? (you might want to squash your commits to make that easier to do if it makes sense to squash some of them)

I am also thinking of limiting it to Linux (for now) since I saw that windows has some issues with package installation when using uv since it does a better job at finding problems than pip.

That's fine I think.

Limit to linux os for now
Fix Polars hist lower bound tests
Fix test by changing sql.DataFrame to sql.classic.DataFrame
Fix sanitize error to work on CI
@jernejfrank jernejfrank force-pushed the migrate_to_github_acitions branch from ffccf31 to a21f800 Compare June 18, 2025 06:51
@jernejfrank
Copy link
Contributor Author

@skrawcz ok, got the tests sorted out and did a rebase. Let me know if you're happy with squash merge or need me to organize the commits :)

name: "Unit Tests"
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false # will change this to true at the end, but want to see tests failing on all use cases
Copy link
Contributor

Choose a reason for hiding this comment

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

need to flip this?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I think we want this to be false always.

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Let's squash merge this into a single commit. This will make things simpler to rebase that way ; I don't think we need to keep the individual commits, right?

I usually craft a commit message, and then leave the prior squashed commits below it!

@jernejfrank jernejfrank merged commit 3c32aca into apache:main Jun 20, 2025
8 checks passed
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.

2 participants