gjbarnard / moodle-format_grid

Grid course format contributed by Gareth J Barnard originally created by Paul Krix
GNU General Public License v3.0
21 stars 56 forks source link

Optimising 2022072200 upgrade step #195

Closed jay-oswald closed 9 months ago

jay-oswald commented 1 year ago

Hey Gareth got some rough code here that I've written to optimise the upgrade step. We have a client that has over 300k records in the format_grid_icon table, this single upgrade step was taking about 3.5 hours to run, after these changes it takes just under 4 minutes.

I have replaced the initial loop with a single SQL query, which has the same result, this step alone saved around 40 mins of run-time, and stopped memory issues, as loading all 300k+ rows into memory twice does use a reasonable amount of memory.

Then instead of the 2nd loop it adds records to the adhoc table. Because we no longer have the array from step 1 each individual course id fetches its own array. This has 2 benefits, it means the upgrade step happens much quicker, and adhoc tasks also lets multiple tasks run at a time, significantly reducing the time to run. In my testing it was taking about 60 mins to process all the adhoc tasks, of course on more powerful production servers running the upgrade there can be more ran in parallel, even reducing the time taken.

I am aware that this PR is not complete, however at this stage I am just wondering if this is something you would be keen on merging in, or if you do not think it is suitable and I can just keep this change for the large client it is necessary for on my own fork

gjb2048 commented 1 year ago

Dear Jay,

On a personal level I'm intrigued by this and the concept of splitting up the task into lots of smaller tasks that make the process more feasible on large systems. However, on a business level it makes no sense to spend time on it without being funded to do so.

Kind regards,

Gareth

gjb2048 commented 1 year ago

P.S. The upgrade needs to apply to M4.0+ not M4.1+ -> https://github.com/jay-oswald/moodle-format_grid/commit/cd0bf3ebf26b29e88389913fe73d977af85e853a#diff-1ce965709f328b395682ab886cb22387271ab62c922166ea2128cb91c5215f64R29

jay-oswald commented 1 year ago

It is up to you Gareth, I am happy to make it apply to M4.0+, and clean up and rename what you want so that you are happy with it. Though I assume you have your own testing you go through so I understand it will still take up your time. I'll run it through the Moodle linting standards but let me know what else you would want changed if you want to go ahead with the change.

gjb2048 commented 1 year ago

Dear Jay,

Thank you for your reply. I will consider integrating the code. Would your client consider sponsoring the format?

Kind regards,

Gareth

jay-oswald commented 1 year ago

Hey Gareth,

They are they're happy for more of my time to make this improvement and get it contributed for the community, but aren't in a position to provide ongoing sponsorship, if that's what you mean.

gjb2048 commented 1 year ago

It is and / or would they be willing to also fund my time to get it contributed into the community?

jay-oswald commented 1 year ago

At this stage no, however we will make sure the code is thoroughly tested and ready to merge to minimise your time

gjb2048 commented 1 year ago

Dear Jay,

Thanks for being honest with me. I will consider the code on a personal interest level. If I then decide at some point to release it on Moodle dot org, then that will be up to me.

Kind regards,

Gareth

gjb2048 commented 1 year ago

P.S. Out of curiosity, do you know why they asked you to fix the issue and not me?

jay-oswald commented 1 year ago

We are a Moodle Partner, and the change was done for one of our clients as a small part of a bigger project. We do what we can to contribute back to the community any useful changes or improvements that we can

gjb2048 commented 1 year ago

So as a Moodle Partner, would you be willing to financially support the people who write the code?

jwcatau commented 9 months ago

Hi @gjb2048

Just confirming this PR is complete and ready to be merged, and was used without issues for the client's update of 300k+ records as per the update - reducing this upgrade step from 3.5 hours to 3.5 minutes was a pretty good result. Thanks @jay-oswald, much appreciated!

Hoping other users might find this PR useful. Cheers!

gjb2048 commented 9 months ago

Dear @jay-oswald and @jwcatau,

Intriguing. I've merged the code into a separate branch as there are a few elements that I'm not happy about with room for improvement and support of a greater client base.

Pragmatically though, I may or may not decide to do them. For me when writing the upgrade script I was not aware / could not imagine that there would be an instance of Moodle with so many grid courses (and thus images). It would have been better if you had reached out to me and we'd worked on a solution where there was a fair / moral consideration in terms of remuneration for my time rather than the path it has taken with a full rejection of my request to be compensated for taking the time to review the code you'd written. I am fully aware and have appreciation not only on a Moodle API level but a software engineering point of view of the design solution you've come up with and given such knowledge of the problem and circumstances could have come up with my own complete solution. To that end then I believe that we would have collaboratively come up with a better result given our combined experience, especially given my long involvement with the format.

Gareth

danmarsden commented 9 months ago

Hey Gareth! - completely understand the dilemma of supporting code and reviewing pull requests from the community without funding - we share that same problem with many of the plugins we develop and support too - I dislike the "when will this plugin support version X" questions the most myself! I also have a large number of pull requests from the community in the backlog that are just not in a good enough state, or will require a significant amount of time for me to review them which I'm not keen to do for "free". :-)

In the case of this particular PR - we've resolved the issue we had with the client affected - so from our end, we don't need you to include this in your release - as a Moodle Partner - one of the main services we provide to our clients is custom development services - we have service level agreements and hosting contracts with clients that pay us for our time but as an company heavily focused around open-source, we try to give-back to the community by providing the code for any improvements or patches we have back to the upstream developer for their consideration and by releasing some of the plugins we develop for them as open-source - in the case of this particular PR - we've resolved the issue we had with the client affected - so from our end, we don't need you to include this in your release, but wanted to provide the patch to you in case you'd had other requests about it or if you felt it might be useful. Feel free to close this as won't fix - our main aim here was to provide the code in case you felt it would be useful.

In relation to your question around Moodle Partners being able to financially support people that write plugin code - unfortunately that's not something we are able to do directly. We provide (significant) financial support back to Moodle HQ as part of our partnership agreements and try to contribute back as much code to the community as we can, but funding individual developers of plugins that our clients use is not something that would be easy for us to do. Some of our clients might be in a position where they could provide funding to the plugin developers (some clients do with plugins like XP, Moove, Poodll where there is an exisitng commercial model for support but that's a mixed bag.)

I note that Moodle HQ is working on a new funding model to replace MUA and they've made noises about this possibly including something around the plugins database but we are still waiting to see what that might look like later this year - I wish I could better monetize some of my personal plugins too, but I'm yet to see a good way to do that... let me know if you manage to crack that as I'd be interested to see what you come up with too!

all the best!

gjb2048 commented 9 months ago

Dear Dan (@danmarsden),

Thank you for your reply. I agree with your first paragraph. I appreciate your second paragraph and being informed, and given the option to use the solution. With your third paragraph, you're a business, I'm a business, why can't you support people that write plugin code? With your fourth paragraph, sadly it seems that the 'stick' rather than the 'carrot' method appears to working with the Adaptable theme - as indeed I'm in the same dislike boat as you with "when will this plugin support version X".

Kind regards,

Gareth

danmarsden commented 9 months ago

Thanks Gareth - I'll raise it internally - although I can't see an easy route for us to donate $$ to developers of plugins just because our clients have decided to use a plugin - perhaps you could think about other ways that might allow our clients to fund development or feature requests or get some form of benefit through a subscription model or a premium version of the plugin.

We'll continue to send you any pull requests for bug fixes or small bits of feature development that our clients might ask us to do, but keep in mind we're sharing this back as part of our community contributions and there's absolutely no pressure on our side for you to review or integrate any of the fixes or pull requests we send your way. If you manage to create some form of formal subscription or ability for clients to purchase time for reviewing pull requests via some form of credit card payment we could try to include that in our comms with any clients that want to pay us to improve your plugins in future too - but unfortunately I can't guarantee that it will result in anything substantial coming your way - although I'll be watching to see how successful you are to see if I can reproduce it in some of my own plugins too! :-)