Build incremental components through build workflow with loading previous build manifest #4289

zelinh · 2023-12-14T23:35:41Z

Description

This is the last PR to enable incremental build on the Python workflow.

The idea of this PR is to build those components that were detected by rebuild_component from build_incremental class.

Firstly we add a new parameter to the build_recorder class so that it could load existing build manifest if provided; otherwise generate a brand new manifest.

Then similar to how we build in run_build.py, here for incremental build, we are importing the previous build manifest and only build the list of components from buildIncremental.rebuild_plugins().

Issues Resolved

#4210

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

codecov · 2023-12-14T23:41:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (11f9ea4) 91.18% compared to head (c442dab) 91.27%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4289      +/-   ##
==========================================
+ Coverage   91.18%   91.27%   +0.08%     
==========================================
  Files         189      189              
  Lines        6146     6163      +17     
==========================================
+ Hits         5604     5625      +21     
+ Misses        542      538       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zelinh · 2023-12-15T07:52:29Z

I'm looking to add additional tests to fix the cov.

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh · 2023-12-15T23:03:21Z

PR is ready for review. Seems like some issues with Maven so that the groovy test failed.

src/build_workflow/build_incremental.py

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

dblock

This is pretty clean. See below, rename/simplify. Document this feature?

dblock · 2024-01-03T03:36:16Z

src/run_build.py

+
+        logging.info(f"Build {components} incrementally.")
+
+        build_manifest_data = BuildManifest.from_path(build_manifest_path).__to_dict__()


Rename this to build_manifest and pass around as BuildManifest, rather than a dict.

Thanks. Working on the changes.

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

dblock · 2024-01-03T21:58:06Z

src/build_workflow/build_recorder.py

            self.components_hash: Dict[str, Dict[str, Any]] = {}

+            if build_manifest:
+                build_manifest_data = build_manifest.__to_dict__()
+                self.data = build_manifest_data


I believe you should be able to iterate through components without turning the build manifest into a dictionary first, which would remove the need for the build_manifest_data variable.

I think that would require to change the logics in other sections for example to_manifest from build_recorder since it generates the build manifest based on dictionary. Do you think it's necessary or it may be overcomplicated.

Whatever you need to add to build_manifest is a good idea. It should look like this here:

self.data = dict(build_manifest) for component in build_manifest.components: self.components_hash[component.name] = dict(component)

or even

self.data = dict(build_manifest) self.components_hash = dict(build_manifest.components)

I see what you mean! Updated in the latest commit. Thanks for suggestion!

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh · 2024-01-03T22:10:22Z

This is pretty clean. See below, rename/simplify. Document this feature?

@dblock Just added some documentations. Please help review it as well. Thanks!

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

src/build_workflow/README.md

src/build_workflow/build_recorder.py

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

src/run_build.py

peterzhuamazon · 2024-01-03T23:44:35Z

Just reviewed with @zelinh on this PR.

I have no issues with the implementation and is clear on the details.
Regarding the conflict between --component and --incremental, is it possible to go these two routes:

If both parameters present, error out the build and provide information in console, that user can only use either parameter, not both at the same time.
If both parameters present, output a warning msg in the console, notify user that --component will not take effect when --incremental is presented at the same time.

The 1st route needs more implementation, while the 2nd route requires one more logging.
Please let me know what you think.

Thanks.

peterzhuamazon · 2024-01-03T23:57:37Z

After discussing again among @gaiksaya @zelinh and I, we have decided to make both --component and --incremental mutually exclusive by following the solution here:
https://github.com/opensearch-project/opensearch-build/blob/main/src/validation_workflow/validation_args.py#L114

Later, we can make both parameters work together, in cases where user want to use --incremental function to verify if there is a change within the component(s) defined by --component parameter.

Thanks.

src/run_build.py

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh · 2024-01-04T00:29:28Z

Updated based on what @peterzhuamazon @gaiksaya and I discussed earlier.
I make --components and --incremental mutually exclusive for now. We can discuss about the future enhancement later.
If the rebuild list is empty, the whole build process will stop and skip building from everything.

Please help review it again. Thanks! @peterzhuamazon @gaiksaya @dblock

gaiksaya

Almost there!

Please add test for skipping the entire build and ensure the graceful exit.
Thanks!

src/run_build.py

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh · 2024-01-04T00:55:05Z

Almost there!

Please add test for skipping the entire build and ensure the graceful exit. Thanks!

Added new tests. I tried locally and it works.

[opensearch@c81bdd48ecdb opensearch-build]$ ./build.sh manifests/2.12.0/opensearch-2.12.0-inc.yml --incremental
Installing dependencies in . ...
Installing dependencies from Pipfile.lock (fcf5b2)...
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
Running ./src/run_build.py manifests/2.12.0/opensearch-2.12.0-inc.yml --incremental ...
2024-01-04 00:47:28 INFO     Loading tar/builds/opensearch/manifest.yml
2024-01-04 00:47:29 INFO     Updating ref for https://github.com/opensearch-project/OpenSearch.git from 923aa25367006b830a0b66aca82eb9c47dafba22 to 923aa25367006b830a0b66aca82eb9c47dafba22 (923aa25367006b830a0b66aca82eb9c47dafba22)
2024-01-04 00:47:29 INFO     Updating ref for https://github.com/opensearch-project/common-utils.git from 2.x to 8e2537b6c1cde03449f0818242a1a36089a69522 (refs/heads/2.x)
2024-01-04 00:47:29 INFO     Updating ref for https://github.com/opensearch-project/job-scheduler.git from 2.x to 482c90fccf59a3e02bc5cb60124580c164a9d7c3 (refs/heads/2.x)
2024-01-04 00:47:30 INFO     Updating ref for https://github.com/opensearch-project/geospatial.git from 2.x to 273fa8fe051d469b5a79ee37a1ce203738239ddb (refs/heads/2.x)
2024-01-04 00:47:30 INFO     Updating ref for https://github.com/opensearch-project/security.git from 2.x to 2d7d6702c8aab1fbb25ab660f3432062fa564e10 (refs/heads/2.x)
2024-01-04 00:47:30 INFO     No commit difference found between any components. Skipping the build

gaiksaya

LGTM! Great enhancement @zelinh 👏

peterzhuamazon

Thanks @zelinh , please open the improvement issue related to both parameters when you have time.

Thanks.

zelinh · 2024-01-04T01:33:27Z

Issue created here as we discussed. #4320

zelinh added 3 commits December 13, 2023 15:59

Update build incremental with tests

254809a

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

Rebase from main

87fddd6

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

More tests for run_build class

cbad22f

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh requested review from dblock, peterzhuamazon, bbarani, gaiksaya, rishabh6788, jordarlu, prudhvigodithi, Divyaasm and tianleh as code owners December 14, 2023 23:35

github-actions bot added the distinguished-contributor label Dec 14, 2023

More tests for continue on error

a8d01d0

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh force-pushed the build_incremental branch from 39ef847 to a8d01d0 Compare December 15, 2023 20:49

Merge remote-tracking branch 'upstream/main' into build_incremental

984e79c

Merge remote-tracking branch 'upstream/main' into build_incremental

20b74d4

gaiksaya requested changes Jan 2, 2024

View reviewed changes

src/build_workflow/build_incremental.py Outdated Show resolved Hide resolved

src/build_workflow/build_incremental.py Outdated Show resolved Hide resolved

src/build_workflow/build_incremental.py Outdated Show resolved Hide resolved

zelinh force-pushed the build_incremental branch from 441c604 to 9e890bf Compare January 3, 2024 02:37

Refractoring the incremental building process

225b0ec

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

zelinh force-pushed the build_incremental branch from 9e890bf to 225b0ec Compare January 3, 2024 02:46

dblock reviewed Jan 3, 2024

View reviewed changes

Rename data to build manifest

9534288

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

dblock reviewed Jan 3, 2024

View reviewed changes

Add documentations

ec61ce5

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

Remove unnecessary variable

142480f

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

gaiksaya reviewed Jan 3, 2024

View reviewed changes

src/build_workflow/README.md Outdated Show resolved Hide resolved

gaiksaya reviewed Jan 3, 2024

View reviewed changes

src/build_workflow/build_recorder.py Outdated Show resolved Hide resolved

Update wording

cb0050c

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

gaiksaya reviewed Jan 3, 2024

View reviewed changes

src/run_build.py Show resolved Hide resolved

peterzhuamazon reviewed Jan 4, 2024

View reviewed changes

src/run_build.py Show resolved Hide resolved

Make components and incremental mutually exclusive

0c9da6b

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

gaiksaya reviewed Jan 4, 2024

View reviewed changes

src/run_build.py Outdated Show resolved Hide resolved

src/run_build.py Show resolved Hide resolved

Add more tests

c442dab

Signed-off-by: Zelin Hao <zelinhao@amazon.com>

gaiksaya approved these changes Jan 4, 2024

View reviewed changes

peterzhuamazon approved these changes Jan 4, 2024

View reviewed changes

zelinh merged commit 810f742 into opensearch-project:main Jan 4, 2024
13 checks passed

zelinh deleted the build_incremental branch January 4, 2024 01:29

zelinh self-assigned this Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build incremental components through build workflow with loading previous build manifest #4289

Build incremental components through build workflow with loading previous build manifest #4289

zelinh commented Dec 14, 2023

codecov bot commented Dec 14, 2023 •

edited

Loading

zelinh commented Dec 15, 2023

zelinh commented Dec 15, 2023

dblock left a comment

dblock Jan 3, 2024

zelinh Jan 3, 2024

dblock Jan 3, 2024

zelinh Jan 3, 2024

dblock Jan 3, 2024 •

edited

Loading

zelinh Jan 3, 2024

zelinh commented Jan 3, 2024

peterzhuamazon commented Jan 3, 2024

peterzhuamazon commented Jan 3, 2024 •

edited

Loading

zelinh commented Jan 4, 2024

gaiksaya left a comment

zelinh commented Jan 4, 2024

gaiksaya left a comment

peterzhuamazon left a comment

zelinh commented Jan 4, 2024


		logging.info(f"Build {components} incrementally.")

		build_manifest_data = BuildManifest.from_path(build_manifest_path).__to_dict__()

Build incremental components through build workflow with loading previous build manifest #4289

Build incremental components through build workflow with loading previous build manifest #4289

Conversation

zelinh commented Dec 14, 2023

Description

Issues Resolved

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

zelinh commented Dec 15, 2023

zelinh commented Dec 15, 2023

dblock left a comment

Choose a reason for hiding this comment

dblock Jan 3, 2024

Choose a reason for hiding this comment

zelinh Jan 3, 2024

Choose a reason for hiding this comment

dblock Jan 3, 2024

Choose a reason for hiding this comment

zelinh Jan 3, 2024

Choose a reason for hiding this comment

dblock Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

zelinh Jan 3, 2024

Choose a reason for hiding this comment

zelinh commented Jan 3, 2024

peterzhuamazon commented Jan 3, 2024

peterzhuamazon commented Jan 3, 2024 • edited Loading

zelinh commented Jan 4, 2024

gaiksaya left a comment

Choose a reason for hiding this comment

zelinh commented Jan 4, 2024

gaiksaya left a comment

Choose a reason for hiding this comment

peterzhuamazon left a comment

Choose a reason for hiding this comment

zelinh commented Jan 4, 2024

codecov bot commented Dec 14, 2023 •

edited

Loading

dblock Jan 3, 2024 •

edited

Loading

peterzhuamazon commented Jan 3, 2024 •

edited

Loading