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.

Sign up for GitHub

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

Jump to bottom

perf(cli): avoid unnecessary module resolution when filtering by name #6002

Merged
merged 10 commits into from
May 14, 2024
Merged

perf(cli): avoid unnecessary module resolution when filtering by name #6002

merged 10 commits into from
May 14, 2024

Conversation

edvald
Copy link
Collaborator

@edvald edvald commented May 6, 2024 •
edited by vvagaytsev
Loading

This applies to most common usages of the build, deploy, run and test commands when one or more names are specified, as well as in the get modules and get actions commands.

For now this is enabled specifically by setting the GARDEN_ENABLE_PARTIAL_RESOLUTION=true env variable.

Fixes #5844

@edvald edvald requested a review from vvagaytsev May 6, 2024 21:35
@edvald edvald force-pushed the partial-module-resolution branch from 668abf1 to 51cb7a9 Compare May 6, 2024 21:41
vvagaytsev
vvagaytsev reviewed May 7, 2024
View reviewed changes
core/src/commands/test.ts
Comment on lines +160 to +162
if (opts.module) {
actionsFilter = [...(actionsFilter || []), `test.${opts.module}-*`]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like garden run command also support --module option. Should we implement the same logic for the Run actions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, didn't notice that. I'd say not strictly necessary for that one, feels more relevant for tests (since it's more natural to run all tests for a module than to run all tasks).

vvagaytsev
vvagaytsev previously approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you! 👍

The ModuleResolver.resolve() seems go be a bit long and complicated, but let's avoid unnecessary refactoring for now. The tests pass, so we can go ahead :)

@edvald
Copy link
Collaborator Author

edvald commented May 7, 2024

The ModuleResolver.resolve() seems go be a bit long and complicated, but let's avoid unnecessary refactoring for now. The tests pass, so we can go ahead :)

Fully agree on that one. Since modules are deprecated though, I didn't want to spend too much time on that. If we put more work into it we can clean it up.

@thsig thsig force-pushed the partial-module-resolution branch from 0e441af to 2785f95 Compare May 8, 2024 08:40
edvald and others added 3 commits May 13, 2024 14:09
@edvald @vvagaytsev
perf(cli): avoid unnecessary module resolution when filtering by name
654086c 
This applies to most common usages of the `build`, `deploy`, `run` and `test`
commands when one or more names are specified, as well as in the
`get modules` and `get actions` commands.

For now this is enabled specifically with by setting the
`GARDEN_ENABLE_PARTIAL_RESOLUTION=true` env variable.
@edvald @vvagaytsev
perf: extend partial resolution to action graph resolution
7200ebb
@vvagaytsev
chore(test): fix compile-time errors
754030e 
Post-rebase fix
@vvagaytsev vvagaytsev force-pushed the partial-module-resolution branch from 2785f95 to 754030e Compare May 13, 2024 12:12
@vvagaytsev
Copy link
Collaborator

vvagaytsev commented May 14, 2024 •
edited
Loading

@edvald I reverted one behavioural change in 269b79a (see the commit message
for details). Let's ship it as a separate feature PR.

Please take a look. If you're fine with that, let's merge this.

I'm also thinking about some tiny refactoring to unify the build/run/test/deploy action lookups by names/wildcards and the relevant error handling.

Update. I've just noticed that some tests failed after the commit above.

Well, we can preserve the old behaviour, but fail with different error messages. That should be ok.

@vvagaytsev vvagaytsev requested a review from thsig May 14, 2024 09:13
@vvagaytsev vvagaytsev force-pushed the partial-module-resolution branch 2 times, most recently from 269b79a to f93125f Compare May 14, 2024 09:36
@vvagaytsev
Copy link
Collaborator

Did more precise error handling in 4b86e05. Let's see if all the tests pass.

@vvagaytsev
Copy link
Collaborator

Changes from 4b86e05 caused even more test failures, I'll revert that commit.

@vvagaytsev vvagaytsev force-pushed the partial-module-resolution branch from 84f3c51 to 1e64797 Compare May 14, 2024 10:08
@vvagaytsev
Copy link
Collaborator

Finally, did some explicit check in 1e64797 to ensure the old behaviour when GARDEN_ENABLE_PARTIAL_RESOLUTION=false.

@vvagaytsev vvagaytsev added this pull request to the merge queue May 14, 2024
Merged via the queue into main with commit 86c885f May 14, 2024
41 checks passed
@vvagaytsev vvagaytsev deleted the partial-module-resolution branch May 14, 2024 15:57
@linear linear bot mentioned this pull request May 15, 2024
Closed
This was referenced Jun 3, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vvagaytsev vvagaytsev vvagaytsev approved these changes

@thsig thsig Awaiting requested review from thsig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve project init performance
2 participants
@edvald @vvagaytsev