-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add kedro package
as a CI check
#236
Conversation
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Do you mind also adding this in the github actions workflow - https://github.com/kedro-org/kedro-starters/blob/main/.github/workflows/all-checks.yml so it runs in the CI |
ah, yes, sure |
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Wondering if there's an easier way to setup Hadoop on Windows, maybe through conda (or better, micromamba)? According to conda-forge/pyspark-feedstock#34 (comment), name: kedro-pyspark
channels:
- conda-forge
dependencies:
- findspark
- openjdk
- pyspark
- python=3.11 The fact that you managed to get Hadoop.exe installed through PowerShell is truly impressive, but I doubt Windows users would go that far, and therefore it would maybe be more useful if the way we do things in our CI is representative of how users get to install Kedro. (Not to mention that I highly doubt anybody is trying to use Kedro + PySpark on Windows natively, but that's another story) |
(I don't intend to block the PR over this though, it was more of an informative comment - "if it works, don't touch it") |
Since we use that approach to run other e2e tests as well, maybe we should address this in a separate PR?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think starters CI needs an update anyway (also need to update actions versions) we can do it separately.
@astrojuanlu, @ankatiyar - separate issue opened for updating the whole CI for starters: #237 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the Windows + Hadoop comment because I was passing by but please always take my comments on CI and infra work as suggestions and with a full bucket of salt 😊 Leaving the spot for others to do the final approval 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Motivation and Context
Solves kedro-org/kedro#3209
How has this been tested?
make install-test-requirements
behave
- to run all tests orbehave -n <Scenario>
to run specific testChecklist