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

Fix WINDOW CI PYTHON 3.10 fail pip-compile #1639

Closed
wants to merge 1 commit into from

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Jun 21, 2022

Signed-off-by: Nok Chan [email protected]

Description

Fixed fail CI due to pip-compile at #1630

PEP517 and PEP518 are relatively new specifications related to the Python build system. It causes some issue with some of the libraries that we depends on ob.

Development notes

Checklist

  • Read the contributing guidelines
  • 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 RELEASE.md file
  • Added tests to cover my changes

@noklam noklam marked this pull request as ready for review June 21, 2022 11:43
@noklam noklam requested a review from idanov as a code owner June 21, 2022 11:43
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.

Nice find! 💪 Any idea why it only complained on windows and not on unix?

@noklam
Copy link
Contributor Author

noklam commented Jun 21, 2022

@MerelTheisenQB Not really sure about this, it could be caused by the dependencies, so maybe some of them didn't support Window Python 3.10 well.

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@deepyaman
Copy link
Member

deepyaman commented Jun 21, 2022

@noklam More because I don't understand PEP 517/518 particularly well, but I don't understand how this resolves the issue. If you look at the pip install logs for this build, you'll see that import-linter is being built based on pyproject.toml rather than PEP517--could this be something that is inconsistent because import-linter doesn't specify their build system?

I don't quite understand how changing the build system for Kedro here made a difference. I think the behavior here is honestly equivalent to what's already happening, because it doesn't significantly deviate from the fallback build system.

In summary:

  • It seems these failures are intermittent, and maybe just got lucky on this build? ;)
  • Maybe it's import-linter that needs the specified build backend; no idea why it's coming up sometimes as PEP517 and other times as pyproject.toml.

Anyway, could also be totally off about this, since I've just been learning more about it since last week. :)

@deepyaman
Copy link
Member

@noklam
Copy link
Contributor Author

noklam commented Jun 21, 2022

@deepyaman Could you try re-trigger your PR to see if it will pass? To be honest, I don't understand the issue fully as well, I have noticed my PR fail with the pip-compile test occasionally and it's more stable when I have the pyproject.toml.

I think the problem PEP517 and PEP518 are trying to solve is the chicken or egg problem - What versions of setuptools and wheel does one need to need to read the setup.py? Currently there is a circular dependencies, and I have no idea how pip or setuptools are handling this, it's highly likely depends on the version too.

@deepyaman
Copy link
Member

@deepyaman Could you try re-trigger your PR to see if it will pass? To be honest, I don't understand the issue fully as well, I have noticed my PR fail with the pip-compile test occasionally and it's more stable when I have the pyproject.toml.

https://app.circleci.com/pipelines/github/kedro-org/kedro/9592/workflows/ca4978bd-38cd-47ec-a1d8-b9adf3eee257/jobs/146155 is re-running now, and it looks like it built the import-linter (and memory-profiler) wheels without issue.

@deepyaman
Copy link
Member

I've got a new idea to deal with the failing pip-compile checks--delete them. I was wondering why we need to pip-compile and not use the result. Diving in:

  1. 4f02f5d
  2. https://jira.quantumblack.com/browse/KED-1254
  3. https://github.com/quantumblacklabs/private-kedro/pull/329#discussion_r352075743

(last two links are internal; sorry, public people)

In short, there used to be issues guaranteeing globally-resolvable requirements. Since then (the issue is from 2019), pip's dependency resolver has changed and isn't complete 💩. I personally don't think there would be an issue with just removing the pip-compile steps now, especially given the fact that pip-compile doesn't support the new pip resolver--yet.

@antonymilne
Copy link
Contributor

antonymilne commented Jun 23, 2022

@deepyaman That is a radical thought! It does indeed seem that the pip-compile jobs are there largely for historical reasons, but I have found them quite useful for immediately flagging where there's version conflicts and the like. I guess this would still be flagged by the pip install -r test_requirements.txt stage of unit tests though. Given that we have essentially removed pip-compile from users' install flow (it was originally in there due to lack of pip dependency resolver) it would be consistent if we also didn't use it any more.

Two things to mention:

  1. We should think about our general policy on upper bounding and whether we should use dependabot (separate issue, but somewhat related as it could reduce the number of conflicts)
  2. Handling dependencies should be much easier when we move kedro dataset dependency out of this repo and into the new kedro-datasets one

@noklam
Copy link
Contributor Author

noklam commented Jun 23, 2022

For point 2, it will be easier for the kedro main repository, but I guess it just shifts the problem to the kedro-datasets repo?

@deepyaman
Copy link
Member

I guess this would still be flagged by the pip install -r test_requirements.txt stage of unit tests though.

@AntonyMilneQB this was my understanding!

@noklam noklam marked this pull request as draft June 30, 2022 16:25
@noklam noklam closed this Sep 7, 2022
@noklam noklam deleted the fix/window-ci-build-py310 branch September 7, 2022 10:25
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.

None yet

5 participants