search

moodlehq / moodle-mlbackend-python

Moodle machine learning backend
GNU General Public License v3.0
18 stars 19 forks source link

how to deal with multiple pull requests from catalyst/moodle-mlbackend-python #26

Open douglasbagnall opened 4 years ago

douglasbagnall commented 4 years ago

This is in reference to #21, #22, #23, #24, #25, and several more that you haven't seen yet.

Please don't merge these in a hurry. There is a narrative order that would suit these changes best, for which I intend to make a mega-pull request, but I am pulling them out into smaller parcels first to sort them out and make them easier to review.

this work makes moodle-mlbackend-python:

along with miscellaneous bug fixes and improved input validation.

there is one point at which the save format on disk changes, which should probably be marked as a major version break. People who need to keep using the old format can stick with 2.x, while everyone else can use 3.x.

So how should I proceed? Lay out a few more partial merge requests, followed by a concatenation of them all in what I think is the best order? (that would be, roughly, tests first, changes breaking disk format or dependencies last last).

douglasbagnall commented 4 years ago

please look at #24, #27, and #28.

stronk7 commented 4 years ago

Hola @douglasbagnall ,

first of all, many thanks for all the stuff that you're incorporating to the python backend, great stuff!

Then, an apology because of the delay replying to this. As you may know, our ML expert, @dmonllao moved away from Moodle HQ and now we are in the transition of getting one of our development teams in charge of all this area. Hopefully that will be sorted soon.

In the mean time, I'm happy to discuss / review / object ... about all this stuff advancing as much as possible. Note I'm a ML / analytics illiterate, so don't expect from me too much technical discussion (I'll basically will trust you), but just check that all the stuff is safe enough, analyze which Moodle versions are good targets for it and ensure that all the versioning / branching / packages are rebuilt properly.

Of course, (I've seen that you are also introducing some testing), the more we can have covered in an automated way, the better. Yuppi!

And, about branching... as you comment, yes. Right now we are un the version 2.x of the packages, that can be used both by Moodle 3.8.x and Moodle 3.9 (dev). And, while we keep the changes BC... there isn't need to bump the major.

So, surely... and aiming to get as much as possible within 2.x... I'd propose to start with the PRs that are 100% safe to be used by both Moodle 3.8 and 3.9. Then, once we start changing things, breaking BC in any way, jump.

Let's go!

stronk7 commented 4 years ago

So, just to ensure which could be the order for all this... we have:

Is that correct? Should we start in that order?

Edited: "close-able" because I've seen that #23 has not been closed (no matter it's included into #27).

douglasbagnall commented 4 years ago

that is correct, though #21 is also in #27 (and thus also #28).

29 should apply on top of #28.

I think the best order would be to start with #24, since it changes very few existing files and it adds tests which go some way towards ensuring the correctness of following patches. Then #27, then #28.

29 can go at any point after #24.

I see on #21 you make a good point about 60da839 possibly breaking some installs. I will shift that to later in the series -- I guess making #28 the breaking changes PR.

thanks!

douglasbagnall commented 4 years ago

I have seen some things I want to fix. For example, there's a "WIP" commit message, and I want to put my benchmark script into the series.

I'll get onto that tomorrow.

douglasbagnall commented 4 years ago

OK, so now I have #30 and #31, which conceptually map to 2.5 and 3.0 respectively.

I don't know whether it preferred to keep making new PRs like this or to force push over the old branches.

douglasbagnall commented 4 years ago

now these have hooks to run the tests on upon pushing to github.

danmarsden commented 3 years ago

@ilyatregubov - this one can be closed now too! - thanks! :-)