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

Replace composer.canasta.json with YAML additions #463

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

naresh-kumar-babu
Copy link
Contributor

Fixes #460 .

Copy link
Member

@jeffw16 jeffw16 left a comment

Choose a reason for hiding this comment

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

The current implementation is inefficient and needs to be rethought.

_sources/scripts/extensions-skins.php Outdated Show resolved Hide resolved
exec($gitApplyCmd);
if ($additionalSteps !== null) {
foreach ($steps as $additionalSteps) {
if ($steps === "composer update"){
Copy link
Member

Choose a reason for hiding this comment

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

Is it really a good idea to run composer update once per extension? Shouldn’t these changes be applied all at once and then composer update be run once at the very end? I can’t support an O(n) implementation for what could easily be an O(1) operation.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to run "composer update" once per extension/skin, yes - it's less hacky, and allows for more flexibility. (And technically, I think it's O(n) either way, though I wouldn't be surprised if doing it all via a single Composer JSON file, i.e. the current way, is indeed faster.)

Copy link
Member

Choose a reason for hiding this comment

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

The difference is, there’s a lot of one-time setup and breakdown when you call composer update. So it takes longer to call composer update 10 times, one update per call, than to call it once with 10 updates.

Copy link
Member

Choose a reason for hiding this comment

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

I can’t in good conscience approve this current implementation.

@yaronkoren
Copy link
Member

@jeffw16 - could you put your concerns (even if it's just a copy of what you wrote here) on the main issue page, #460? That seems like the better place to talk about it.

@jeffw16
Copy link
Member

jeffw16 commented Dec 12, 2024

@yaronkoren: This is an implementation issue with this PR, not a general problem with the proposal, so I don’t see the point of doing that.

@yaronkoren
Copy link
Member

Not really... I think the whole issue of whether to keep the current "composer.canasta.json" setup is part of the design aspect. But if you really want to, we can talk about it here.

@jeffw16
Copy link
Member

jeffw16 commented Dec 12, 2024

That’s the thing - I don’t have a problem with doing away with “composer.canasta.json”, but I do have a problem with the way this current implementation addresses it. In essence, I am just asking for this current attempt to be refactored so composer update is called just once.

@yaronkoren
Copy link
Member

I think the only way that can be done is by generating composer.local.json on the fly, and then calling "composer update" on it. Which is certainly doable, and I thought about going with that approach. I still think this approach is better, though, because it just feels like less of a hack - there's one less "moving part", and we're calling Composer the way it's meant to be called. (It's notable that the Composer merge plugin, as useful as it is, is a Wikimedia invention.) If there's a problem with one of the Composer JSON files, it would presumably be easier to detect if they're being called one at a time. And I don't think performance is a real issue - assuming this approach is a few seconds slower, it will have no real impact on the user experience.

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

I'm getting very nervous about these small changes being made here and there while this discussion is ongoing. This indicates when the PR was initially released, the code was not complete. By definition, the PR should only be created when the code is completely ready to be merged.

Could we have complete code coverage for all of these changes please? I expect to see comprehensive test cases with PHPUnit before I give my approval.

@yaronkoren
Copy link
Member

Sorry, I don't understand this. Changes shouldn't be made to a PR? You've requested changes to this PR yourself.

And why does this PR require a test script, while no previous one did? Not that I'm against test scripts - I agree it would be great to have them.

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

You've requested changes to this PR yourself.

That's different. Making changes in response to comments is very different from requesting a review and then continuing to make nontrivial changes to achieve code completion.

And why does this PR require a test script, while no previous one did?

This is a logical fallacy. Just because we didn't follow good practice in the past doesn't give us an excuse to keep this practice going.

Why am I now requesting they be included? Because the code here clearly wasn't code complete when the PR was published, and I've lost my trust in the quality of the code that's being delivered. If the PR wasn't ready for review, then it should've been kept in draft mode and nobody should have been requested to review it.

This was not an issue in the past, but it has gotten to the point where it's become an issue I can't overlook anymore.

In summary, you can consider this the equivalent of the FAA no longer giving Boeing blanket approvals for 737 MAX certifications.

@yaronkoren
Copy link
Member

Okay, thanks for the explanations. I'm pretty sure what I wrote wasn't a logical fallacy, but that's kind of a minor point.

@yaronkoren
Copy link
Member

@naresh-kumar-babu - since performance is still a concern: could you run this code again, and time how long it takes for all the extensions and skins to be installed? (Or, if it's easier, how long the entire process takes?)

@naresh-kumar-babu
Copy link
Contributor Author

@naresh-kumar-babu - since performance is still a concern: could you run this code again, and time how long it takes for all the extensions and skins to be installed? (Or, if it's easier, how long the entire process takes?)

Time taken by existing approach: 258s
Time taken by proposed approach: 230s

@yaronkoren
Copy link
Member

@naresh-kumar-babu - that's very unexpected! I certainly would never have guessed that calling "composer update" multiple times reduces the execution time. Do you have any idea why this is the case? Could it be that the "merge" plugin runs slowly, due to the complexity of handling everything at once? Or maybe this is just a fluke outcome, because the execution time varies each time?

@yaronkoren
Copy link
Member

Actually, another question: is this just the time taken to install extensions and skins, or the time for the overall setup?

@naresh-kumar-babu
Copy link
Contributor Author

Actually, another question: is this just the time taken to install extensions and skins, or the time for the overall setup?

Time taken to install extensions and skins.

@naresh-kumar-babu
Copy link
Contributor Author

@naresh-kumar-babu - that's very unexpected! I certainly would never have guessed that calling "composer update" multiple times reduces the execution time. Do you have any idea why this is the case? Could it be that the "merge" plugin runs slowly, due to the complexity of handling everything at once? Or maybe this is just a fluke outcome, because the execution time varies each time?

I'm not sure. I can run a few more times and verify the timing.

@yaronkoren
Copy link
Member

Yes - if you could run each of them at least one more time, that would be great.

@naresh-kumar-babu
Copy link
Contributor Author

Yes - if you could run each of them at least one more time, that would be great.

1st Attempt:
Existing approach: 252s
Proposed approach: 227s

2nd Attempt:
Existing approach: 240s
Proposed approach: 230s

@yaronkoren
Copy link
Member

That's awesome! I'm glad you ran these tests - if we make this change, now we'll never have to wonder whether "the tradeoff was worth it", because it turns out that there is no tradeoff - maybe the "merge" plugin is poorly designed or something, but in any case, this new approach is both easier to understand and faster.

This also seems like a good test of the overall code - you have managed to run it consistently without problems.

@jeffw16 - what do you think?

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

Can Naresh please clarify what he means by “existing approach” versus the “proposed approach”? Does the existing approach refer to what is in the main branch or the coded approach here in the PR?

@naresh-kumar-babu
Copy link
Contributor Author

Can Naresh please clarify what he means by “existing approach” versus the “proposed approach”? Does the existing approach refer to what is in the main branch or the coded approach here in the PR?

Existing approach is what is in the main branch.

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

Okay, thanks. In that case, it looks like what I had proposed was not yet tested. As this represents an incremental improvement, I am okay with this, but can you add the following comment at the for loop for future reference so we can speed things up more in the future?

// TODO: Refactor approach so `composer update` is only called once after the composer.json is dynamically built. 

@yaronkoren
Copy link
Member

Wait - why do you think that approach would be faster? My guess was that the removal of the Composer "merge" was what led to the speedup.

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

I think the removal of the Composer Merge plugin plus refactoring it to call composer update just once will result in even more speed improvements than what we see right now.

I’m happy with the current speed improvements - well done - but we should strive for better in the future. Please add the TODO and I will approve this PR.

@yaronkoren
Copy link
Member

But the alternate approach, of creating a composer.local.json file on the fly, would require using the merge plugin again, no? I would think it would take roughly the same amount of time to run as the current (i.e., with composer.canasta.json) approach.

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

No, just get rid of the composer.local.json file and edit the composer.json file.

@yaronkoren
Copy link
Member

Oh, I get it. But that just adds more complexity, no? That is, more things could go wrong. What's the benefit?

@jeffw16
Copy link
Member

jeffw16 commented Dec 13, 2024

Calling composer update n times doesn’t seem like the most efficient solution.

Please, can we stop this meaningless discussion and just add this TODO so we can get this merged in? It is not difficult to add this one comment to the code, and we’re not bound to implementing it either. We can discuss later whenever someone gets the chance to work on this improvement.

@yaronkoren
Copy link
Member

Well, I didn't think calling "composer update" 15+ times would be the most efficient solution either, but apparently there's a chance that it is...

I just don't want a "TODO" for something that I don't think makes sense to add. Of course, I could be wrong, but I don't want to put in something that indicates some kind of current consensus. If you think this change should be made, the best approach is to create an issue for it. (Or PR, for that matter.)

@yaronkoren
Copy link
Member

Well, "consider change" is certainly better than "do change", but I still don't see the need for this TODO...

Modifying composer.json means modifying the core MediaWiki code, which Canasta tries its best not to do. (I can't remember if Canasta modifies anything, but if so it's minimal.) I don't see how even, say, 60-second speedup of this process would be worth the added complexity and potential confusion - and there's no way that a change to the Composer handling would reduce the time by 60 seconds. (The majority of the extension and skin install time is presumably just fetching all the code via Git.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace composer.canasta.json with YAML additions
3 participants