Skip to content

MNT Pull main on feature/engine-api dev branch #26949

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

Merged
merged 666 commits into from
Aug 4, 2023

Conversation

fcharras
Copy link
Contributor

What does this implement/fix? Explain your changes.

git pull --no-rebase origin main on feature/engine-api branch with conflict resolution.

rprkh and others added 30 commits May 17, 2023 10:46
)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…arn#26019)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
…n#26016)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…rn#26124)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
…entroid (scikit-learn#24083)

Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
…earn#25429)

Co-authored-by: ArturoAmorQ <arturo.amor-quiroz@polytechnique.edu>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
…ly (scikit-learn#15554)

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
…cikit-learn#26318)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@adrinjalali
Copy link
Member

I don't think this PR belongs here.

If you're trying to update your local branch, refer to https://scikit-learn.org/dev/developers/contributing.html#how-to-contribute

@fcharras
Copy link
Contributor Author

fcharras commented Jul 31, 2023

This isn't a mistake, the branch https://github.com/scikit-learn/scikit-learn/tree/feature/engine-api has been moved from @ogrisel fork to scikit-learn main repo but I lost permissions to push on it in the process.

@fcharras fcharras changed the title MNT Puill main on feature/engine_api dev branch MNT Pull main on feature/engine-api dev branch Jul 31, 2023
@adrinjalali
Copy link
Member

This PR is adding more than 46k lines of code, no way we can review it. For feature branches, it's easier for the champion of the branch (I guess in this case @ogrisel ) to merge main into that branch directly every now and then, like the other branches we've had.

@fcharras
Copy link
Contributor Author

We haven't pulled main for several months because the project was slowed down, so now that I want to catch up there's 46k diff. I need the test pipelines to run so I can finalize the work.

The review of the actual diff from feature/engine-api to main is expected to take place in #25535 .

@adrinjalali adrinjalali reopened this Jul 31, 2023
@adrinjalali adrinjalali changed the title MNT Pull main on feature/engine-api dev branch [NOMERGE] MNT Pull main on feature/engine-api dev branch Jul 31, 2023
@fcharras
Copy link
Contributor Author

Thanks @adrinjalali

@adrinjalali
Copy link
Member

Opened the PR for the CI to run, but I would rather not have this PR merged and once tests are okay, @ogrisel can merge main into the feature branch.

@glemaitre
Copy link
Member

@betatim could also merge this branch.

I had a quick look because the 46K lines seem strange but it looks legit.
I did not think that we could have that many changes but it seems that between the isort and lock files, we can get there.

@betatim
Copy link
Member

betatim commented Aug 2, 2023

What is the downside to using a PR based workflow (that targets the feature branch) to work around permission issues like Franck has? Maybe adding to the PR description that this is a work around for permission issues and other context would be useful for the future.

I think we need a way for people like Franck to make progress on work even when all the core devs related to the feature branch are on holiday (for several weeks). And even when I'm not on holiday it is nice for Franck to be able to do stuff, instead of being blocked by when I/Olivier have time to get around to something. Maybe we can work out a way to achieve that.

(I've not yet had time to look through all my GH notifications since going on holiday)

@fcharras
Copy link
Contributor Author

fcharras commented Aug 2, 2023

At the pace we've been working on this branch lately it's not too much of an issue. For now my work on this branch is completed (fully tested with soda-inria/sklearn-numba-dpex#114 ) could you synchronize it with engine/feature-api please ? (merge or manually pull on engine/feature-api, as you wish)

There were only a few minor conflicts in kmeans (with recent changes regarding sample_weight) + ruff compliance to catch on.

@adrinjalali
Copy link
Member

What is the downside to using a PR based workflow (that targets the feature branch) to work around permission issues like Franck has? Maybe adding to the PR description that this is a work around for permission issues and other context would be useful for the future.

The downside is that this came out of nowhere, with no context given on the PR description. So I genuinely thought it's a mistake / spam PR. And it's rebasing and including like 180 PRs or more. There's no way to review this.

I think we need a way for people like Franck to make progress on work even when all the core devs related to the feature branch are on holiday (for several weeks). And even when I'm not on holiday it is nice for Franck to be able to do stuff, instead of being blocked by when I/Olivier have time to get around to something. Maybe we can work out a way to achieve that.

When I worked on SLEP6, other people were working on it as well, but I made sure the branch is up to date whenever it needed to be. That's what I'd expect from you (@betatim ) or @ogrisel to do as well. And you not doing it in time doesn't block people from working on the feature since everybody can merge with main and work on their corresponding feature while you manage to update the upstream branch. So letting the champions do this isn't really a blocker.

But sure, if you wanna merge this into the feature branch go ahead. However, it would be nice if one of you handles these cases.

@adrinjalali adrinjalali changed the title [NOMERGE] MNT Pull main on feature/engine-api dev branch MNT Pull main on feature/engine-api dev branch Aug 2, 2023
@betatim
Copy link
Member

betatim commented Aug 4, 2023

Merging this for now.

I agree that we need more context in the original message if we want to use this strategy in the future.

@betatim betatim merged commit 1915869 into scikit-learn:feature/engine-api Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.