-
Notifications
You must be signed in to change notification settings - Fork 60
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 Windows CI to kedro-starters #89
Conversation
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
* Try cloning env in before scenario Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try again Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try find line Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try find line 2 Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try find line 3 Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try without venv.main step Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try activating env at every windows build step Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Activate env before running tests/linting Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Clean up environment Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try adding hadoop setup step Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Try adding hadoop setup step Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Clean up Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> * Make formatting consistent Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> Co-authored-by: Merel Theisen <merel.theisen@quantumblack.com>
The test for running pyspark-iris fails for the windows builds, the error messages have been captured and are appended below. However, perhaps this should be addressed in a separate issue, as I would say it falls out of scope for the current one. --
|
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.
Great work @AhdraMeraliQB ! I'm happy with skipping the pyspark-iris
test on Windows. Ideally we would add in a bit of logic so the builds pass and you don't need an admin to merge. Basically a check that says "if its the pyspark-iris test and running windows, skip the test". See: https://stackoverflow.com/questions/36482419/how-do-i-skip-a-test-in-the-behave-python-bdd-framework and https://github.com/kedro-org/kedro/blob/main/features/build_docs.feature#L3 + https://github.com/kedro-org/kedro/blob/main/features/environment.py#L41 for how you can add tags to behave test to skip.
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
.circleci/config.yml
Outdated
name: Install Python dependencies | ||
command: | | ||
pip install git+https://github.com/kedro-org/kedro@main | ||
cd package && pip install -r requirements.txt -U |
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.
Where is this package
folder coming from?
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.
Nice catch - looks like we're installing kedro and it's requirements, and then installing them again under the kedro-starters requirements; I'll remove the redundant step.
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.
We might want JDK set up, we could try the win JDK setup we have on the Kedro repo. Having this installed may fix the pyspark-iris issues since it's reliant on Java.
features/environment.py
Outdated
def after_scenario(context, scenario): | ||
rmtree(str(context.temp_dir)) | ||
rmtree(str(context.venv_dir)) | ||
|
||
|
||
def rmtree(top): | ||
if os.name != "posix": | ||
for root, _, files in os.walk(top, topdown=False): | ||
for name in files: | ||
os.chmod(os.path.join(root, name), stat.S_IWUSR) | ||
shutil.rmtree(top) | ||
for path in _PATHS_TO_REMOVE: | ||
# ignore errors when attempting to remove already removed directories | ||
shutil.rmtree(path, ignore_errors=True) |
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.
Is this just refactoring or necessary to make it work? context
and scenario
are now redundant arguments. Does it still belong to after_scenario
or it should be after_all
?
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.
With how behave is set up it always passes the context through, but scenario
was indeed redundant and has been removed. Thanks for the catch!
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.
Thanks for making this work! Leave a couples of small comments.
I also notice on the CircleCI there is a note, on kedro
main repo we don't have this and we can see all the test results within CircleCI UI.
Your output is too large to display in the browser.
Only the last 400000 characters are displayed
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
@SajidAlamQB that's a good shout - will be sure to address in the pyspark-iris issue 👍 |
@noklam I think that might be built into CircleCI for larger outputs; I'm unsure that anything was changed manually to trigger this 🤔 |
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Co-authored-by: Nok <nok.lam.chan@quantumblack.com>
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.
Awesome work @AhdraMeraliQB ! ⭐ 🎉 Great to finally have windows builds for our starters.
Signed-off-by: Ahdra Merali ahdra.merali@quantumblack.com
Motivation and Context
Kedro-starters was lacking Windows CI. This PR rectifies this by adding Windows builds to the e2e testing. The setup has also been changed to make use of conda environments. The tests themselves have not been changed.
Checklist