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.

Sign up for GitHub

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

Jump to bottom

Remove pyyaml as a dependency of Kedro-Telemetry #28

Merged
merged 9 commits into from
May 26, 2022
Merged

Remove pyyaml as a dependency of Kedro-Telemetry #28

merged 9 commits into from
May 26, 2022

Conversation

datajoely
Copy link
Contributor

@datajoely datajoely commented May 17, 2022 •
edited
Loading

Description

Mirror of this PR on core kedro-org/kedro#1541

Development notes

  • See above
  • I think I messed up my git history so have two commits in here, sorry 😓 !

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

AhdraMeraliQB and others added 2 commits May 6, 2022 10:00
@AhdraMeraliQB @yetudada @merelcht
Update the README.md to point to LF AI & Data (#24)
46af4c2 
* Update the README.md to point to LF AI & Data

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Linting

Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>

* Update kedro-telemetry/README.md

Co-authored-by: Yetunde Dada <43755008+yetudada@users.noreply.github.com>

* Update kedro-telemetry/README.md

Co-authored-by: Merel Theisen <49397448+MerelTheisenQB@users.noreply.github.com>

Co-authored-by: Yetunde Dada <43755008+yetudada@users.noreply.github.com>
Co-authored-by: Merel Theisen <49397448+MerelTheisenQB@users.noreply.github.com>
@datajoely
Allow later PyYAML requirements
3665f32
@merelcht
Copy link
Member

I'm not sure you meant to push the changes to the docs?

@datajoely
Copy link
Contributor Author

@MerelTheisenQB I reverted two commits, not one (I didn't realise I was on main) so the doc change went in previously 🙈

@datajoely datajoely requested a review from merelcht May 17, 2022 12:28
merelcht
merelcht approved these changes May 17, 2022
View reviewed changes
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.

👍

kedro-telemetry/RELEASE.md Outdated Show resolved Hide resolved
kedro-telemetry/RELEASE.md Outdated Show resolved Hide resolved
@datajoely datajoely changed the title Kedro telemetry/0.2.2 Kedro telemetry/0.2.1 May 17, 2022
@datajoely
Copy link
Contributor Author

Okay addressed questions - are the e2e tests a known issue?

@antonymilne
Copy link
Contributor

Okay addressed questions - are the e2e tests a known issue?

Hmmm no I don't think so. Is it passing on main or does it give the same error?

@datajoely
Copy link
Contributor Author

@AntonyMilneQB so the I think this is the first time kedro-airflow code has changed in a while, so I'm not sure the CI has run recently

@merelcht merelcht mentioned this pull request May 19, 2022
Merged
3 tasks
@noklam
Merge branch 'main' into kedro-telemetry/0.2.2
e72cc99 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
@noklam
Merge branch 'main' into kedro-telemetry/0.2.2
b7fb437 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor

noklam commented May 25, 2022 •
edited
Loading

The test is failing because it does not pick up the correct kedro module but instead of a user folder in UNIX which called kedro.

  1. python; import kedro will import the correct module
  2. pytest would pick up the incorrect module.

Extra information

I dive into the sys.path and find pytest add lots of stuff to it, this is the direct cause.

See sys.path with pytest

sys.path=['/home', '/usr/local/bin', '/usr/local/lib/python38.zip', '/usr/local/lib/python3.8', '/usr/local/lib/python3.8/lib-dynload', '/usr/local/lib/python3.8/site-packages']

See sys.path when just import normally

sys.path
['', '/usr/local/lib/python38.zip', '/usr/local/lib/python3.8', '/usr/local/lib/python3.8/lib-dynload', '/usr/local/lib/python3.8/site-packages']

This is a bit weird as this test should always fail if it doesn't work, unless pytest make some changes recently causing this.

image

@noklam
Update Username and folder to avoid namespace collision
b672eb2 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
@noklam
Fix user folder
7f3b6d7 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
@noklam noklam requested a review from merelcht May 25, 2022 16:18
@noklam
Update mount volume
30e5ab8 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
merelcht
merelcht approved these changes May 26, 2022
View reviewed changes
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.

Great investigation @noklam ! 🕵️

kedro-docker/project_dummy/Dockerfile Outdated
@@ -0,0 +1,23 @@
ARG BASE_IMAGE=python:3.6-buster
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this Dockerfile?

Copy link
Contributor

Choose a reason for hiding this comment

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

No we don't!

@yetudada yetudada changed the title Kedro telemetry/0.2.1 PyYaml update May 26, 2022
@yetudada yetudada changed the title PyYaml update Remove pyyaml as a dependency of Kedro-Telemetry May 26, 2022
@noklam
Removed the generated Dockerfile
66f1e43 
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
@noklam noklam merged commit dd85fce into main May 26, 2022
@noklam noklam deleted the kedro-telemetry/0.2.2 branch May 26, 2022 13:53
@datajoely
Copy link
Contributor Author

Huzzah!

@szczeles szczeles mentioned this pull request Mar 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noklam noklam noklam left review comments

@merelcht merelcht merelcht approved these changes

@antonymilne antonymilne antonymilne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@datajoely @merelcht @antonymilne @noklam @AhdraMeraliQB