-
Notifications
You must be signed in to change notification settings - Fork 6
[CDF-26465] Rebuild build #2273
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of the task, strictly speaking. Just a proposal from the AI to improve performance through compiling regex'es. Will split into separate task later, please ignore for now.
doctrino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done a first pass and thrown in some comments as I went through. I think I understand the code, but I am not seeing completely were it is heading. But more than good enough to start a discussion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good refactoring that can be moved out to a separate PR.
| from cognite_toolkit._version import __version__ | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use pydantic and not data classes.
| pass | ||
|
|
||
|
|
||
| class BuildWarnings(TypedDict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And avoid TypedDict
|
|
||
| class BuildCommandV2(ToolkitCommand): | ||
| verbose: bool = False | ||
| on_error: Literal["continue", "raise"] = "continue" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a class attribute and an argument to execute?
| # Optimize the build, "ruff and mypy". | ||
| # Consistency checks and recommendations. | ||
| # Fix where we can. | ||
| self._optimize(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call it validate/verify? My pedantic mind with background in optimization, want to maximize or minimize an objective function when I see optimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this take the built_module? as intput
| self._optimize(input) | |
| self._optimize(built_modules ) |
Description
The goal of this change is to improve the performance and overall user experience of the build command. It should also result in more maintainable code. The new killer feature is "optimize" that uses recommendations to suggest and implement improvements to the configuration.
Flow
The general flow aims to collect and validate in stages, building modules in parallel and finally running optimization (i.e. recommendations and fixes) on the built state. It might be better to run optimizations on individual modules, so this may change.
Structure
We'll need a new set of data classes/structure for recommendations. A recommendation will have a unique code so that we can link to docs, but also so that the user can dictate which rules and recommendation that they want to show (yes, like ruff and mypy) in their cdf.toml.
A recommendation, if fixable,
Collecting and logging warnings
Warnings should be collected and grouped by category so that we can print (or write to file) more readable logs;
Exit on warning still applies, but not before warnings have been printed or logged. Exceptions are raised as usual.
Testing
TBD. We'll need to do full regression testing here, in addition to adding new unit tests.
Release
This change will be behind a alpha flag until we decide to release it (planned v0.8)
Bump
Changelog
Added/Changed/Improved/Removed/Fixed