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

ci: Clean up of Makefile and Gitpod setup #464

Merged
merged 12 commits into from
Aug 28, 2024
Merged

ci: Clean up of Makefile and Gitpod setup #464

merged 12 commits into from
Aug 28, 2024

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Dec 7, 2023

Description

Fix the setup, particular kedro-datasets requirements are harder to install for contributors.

Development notes

I decided not to use Docker because of the way Gitpod work, building docker would take up most of the time and the UI does not show up until this is finished.

  • Update to use uv and set the venv by default, so reviewer/contributor can start editing immediately while some heavy requirements are installing in the background.

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@noklam noklam changed the title Update Dockerfile for kedro-datasets ci: Update Dockerfile for kedro-datasets Dec 7, 2023
@noklam noklam marked this pull request as ready for review January 8, 2024 15:51
.gitpod.Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: Nok <[email protected]>
.gitpod.Dockerfile Outdated Show resolved Hide resolved
@merelcht
Copy link
Member

@noklam Do you still want to get this in?

@noklam
Copy link
Contributor Author

noklam commented Aug 23, 2024

@merelcht Sure, let me try to update this is still very useful to review datasets PR. I will update the make command with uv as well.

@noklam noklam changed the title ci: Update Dockerfile for kedro-datasets ci: Clean up of Makefile and Gitpod setup Aug 23, 2024
@noklam noklam marked this pull request as draft August 23, 2024 12:11
@noklam noklam marked this pull request as ready for review August 24, 2024 19:01
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

install-test-requirements:
cd $(plugin) && pip install ".[test]"
cd $(plugin) && uv pip install ".[test]"
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this fixes #597 ?

Makefile Show resolved Hide resolved
.gitpod.yml Outdated Show resolved Hide resolved
init: |
sudo apt-get update && sudo apt-get install -y --no-install-recommends libgl1
sudo apt-get install make
sudo apt-get install -y --no-install-recommends libatk-bridge2.0-0 libcups2 ca-certificates fonts-liberation libasound2 libatk-bridge2.0-0 libatk1.0-0 libc6 libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgbm1 libgcc1 libglib2.0-0 libgtk-3-0 libnspr4 libnss3 libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 libxcursor1 libxdamage1 libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 lsb-release wget xdg-utils
Copy link
Member

Choose a reason for hiding this comment

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

Are all these system deps needed to install stuff on Gitpod? Bummer...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^frankly I didn't test it 1 by 1, this list of dependencies has lived in kedro for a while since the introduction of VideoDataset. opencv seems to relies on some shared libraries.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so it's because of OpenCV, got it 👍🏼

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

I'm not using the Makefile and Gitpod is not my daily driver but since I was requested for review, added some thoughts. Approving nonetheless!

@noklam noklam enabled auto-merge (squash) August 27, 2024 14:48
@noklam noklam disabled auto-merge August 28, 2024 13:21
@noklam noklam merged commit cf1617b into main Aug 28, 2024
52 checks passed
@noklam noklam deleted the fix/gitpod branch August 28, 2024 13:22
harm-matthias-harms pushed a commit to harm-matthias-harms/kedro-plugins that referenced this pull request Oct 1, 2024
* switch to Dockerfile, same configuration as the kedro branch.

Signed-off-by: Nok <[email protected]>

* simplify setup

Signed-off-by: Nok <[email protected]>

* update makefile

Signed-off-by: Nok <[email protected]>

* simplified install in background while not slowing down startup

Signed-off-by: Nok <[email protected]>

* fix command

Signed-off-by: Nok <[email protected]>

* fix setup

Signed-off-by: Nok <[email protected]>

* combined makefile

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok <[email protected]>
Signed-off-by: Harm Matthias Harms <[email protected]>
MinuraPunchihewa pushed a commit to MinuraPunchihewa/kedro-plugins that referenced this pull request Oct 1, 2024
* switch to Dockerfile, same configuration as the kedro branch.

Signed-off-by: Nok <[email protected]>

* simplify setup

Signed-off-by: Nok <[email protected]>

* update makefile

Signed-off-by: Nok <[email protected]>

* simplified install in background while not slowing down startup

Signed-off-by: Nok <[email protected]>

* fix command

Signed-off-by: Nok <[email protected]>

* fix setup

Signed-off-by: Nok <[email protected]>

* combined makefile

Signed-off-by: Nok <[email protected]>

---------

Signed-off-by: Nok <[email protected]>
Signed-off-by: Minura Punchihewa <[email protected]>
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