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

[douglasbagnall]

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:

• faster
• more accurate
• tested (by which we know the first two)
• compatible with tensorflow 1.14 to 2.1(at least)
• more “pythonic”

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]

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

[stronk7]

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]

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

• One side: Misc small improvements #21 (standalone)
• Another side: Add some tests and an INSTALL document #24 , Faster training #27 and Faster, more accurate, future compatible ML with keras  #28 (which are cumulative).
• Finally, Password safety #29 (built on top of 24)
• And, closed/close-able: Switch to 32 bit #22 , Classifier: remove invariant columns #23 and Allow webapp to run without boto3 when not using s3 #25 (all them incorporated to Faster training #27 already).

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]

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]

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]

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]

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

[danmarsden]

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