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

micropkg packaging improvements #2614

Merged
merged 15 commits into from Jun 13, 2023
Merged

Conversation

astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented May 29, 2023

Description

Development notes

All the packaging stuff is quite complex - in fact, despite an initially good streak, this is now my second backtrack attempt, after opening gh-2569 and gh-2597 (hopefully those two can go next).

Keeping the micropackaging workflow intact while removing the strong assumptions around setup.py was extra hard, but I think I managed while staying within the "blessed" way of doing things. I could not avoid a runtime dependency on setuptools for now though, so we'll have to keep it around, maybe as an extra (gh-2350).

It's been mentioned that this workflow is weird and that we should evaluate whether we want to keep it or not https://github.com/kedro-org/kedro/milestone/21 but this work blocks the rest of the progress in https://github.com/kedro-org/kedro/milestone/36, so I didn't want to block it on that decision (which could be taken after 0.19).

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

@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch 2 times, most recently from 829f2db to 8b5087f Compare May 29, 2023 18:46
@astrojuanlu
Copy link
Member Author

A small number of corner cases is failing, will have a look at those later this week.

@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch 3 times, most recently from 20bba12 to f14db1a Compare May 30, 2023 07:38
@astrojuanlu
Copy link
Member Author

The last set of test failures is due to a behavior change from pkg_resources to packaging, noted it here pypa/packaging#644 (comment)

I am hoping that we get an official answer soon, but while we wait, I think this is ready for review.

@astrojuanlu astrojuanlu marked this pull request as ready for review May 30, 2023 08:23
@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch from f14db1a to 2dd7074 Compare May 30, 2023 08:23
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Happy with this from docs perspective, but obvs not the main point of the PR.

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.

I'll need to look at this a second time, because there's quite a lot going on that I still need to process. I've left some initial thoughts/comments, but they're mostly nitpicks 🙈

General question is if this implementation is backwards compatible? (I'll also try it myself when re-reviewing)

kedro/framework/cli/micropkg.py Outdated Show resolved Hide resolved
kedro/framework/cli/micropkg.py Outdated Show resolved Hide resolved
kedro/framework/cli/micropkg.py Show resolved Hide resolved
@astrojuanlu
Copy link
Member Author

I'll need to look at this a second time, because there's quite a lot going on that I still need to process. I've left some initial thoughts/comments, but they're mostly nitpicks 🙈

I totally understand... 2622da6 could be reviewed separately, it's what makes things more complicated here.

I could also split this in 2 PRs if it's easier for folks to review.

General question is if this implementation is backwards compatible? (I'll also try it myself when re-reviewing)

I didn't change any tests (only dropped one that was not needed anymore and modified error messages and pip invokations in others), so in principle this should be backwards compatible.

@astrojuanlu
Copy link
Member Author

Test failures hopefully addressed by https://github.com/pypa/packaging/pull/696/files (although I don't want to wait on that for this PR, we'll probably have to hack around it)

@merelcht
Copy link
Member

merelcht commented Jun 2, 2023

I've tried packaging and pulling with this new version and the old (current) one and it all seems to work fine and to be backwards compatible 👌 It seems like the micropackage that's generated is still exactly the same. I guess that's supposed to be the case right?

@astrojuanlu
Copy link
Member Author

Yes, absolutely! That's why this PR would not fully close gh-2414 - the generated micropkg pretty much still uses setup.py.

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.

This looks like to be working as expected, so I'm happy to have it merged in. Maybe add to the release notes which changes were made here for traceability? And thanks for helping to improve this part of messy code base @astrojuanlu 🙇 ⭐

@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch 4 times, most recently from d4b6005 to fedebbb Compare June 5, 2023 14:22
@astrojuanlu
Copy link
Member Author

I have addressed all the outstanding comments and now I believe all tests should pass. If everything goes well and unless there are other opinions, I'll merge this.

@astrojuanlu
Copy link
Member Author

All tests passing, CI failures due to non-100 % coverage. Will need to add tests to cover the extra possible (but unlikely) error messages.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Fix gh-2542.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
See gh-2414.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Fix gh-2567.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
See pypa/packaging#644 (comment)

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch from fedebbb to f39a024 Compare June 13, 2023 11:40
@astrojuanlu
Copy link
Member Author

Added tests, finally! This is ready to go in. It has 2 approvals already but waiting until EOD to merge.

@astrojuanlu

This comment was marked as resolved.

Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
@astrojuanlu astrojuanlu force-pushed the feat/micropkg-packaging-improvements branch from fde96bb to 5387918 Compare June 13, 2023 12:42
@merelcht
Copy link
Member

Added tests, finally! This is ready to go in. It has 2 approvals already but waiting until EOD to merge.

Ideally another engineer would approve this since the majority of the changes are in code.

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.

Overall, the changes you've made to the micro-packaging workflow seem comprehensive and well thought out. Just some minor comments about readability.

Thank you @astrojuanlu

kedro/framework/cli/micropkg.py Show resolved Hide resolved
Signed-off-by: Juan Luis Cano Rodríguez <juan_luis_cano@mckinsey.com>
@astrojuanlu astrojuanlu enabled auto-merge (squash) June 13, 2023 15:40
@astrojuanlu astrojuanlu merged commit 37277c3 into main Jun 13, 2023
34 checks passed
@astrojuanlu astrojuanlu deleted the feat/micropkg-packaging-improvements branch June 13, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants