Add commits differentiation logic for incremental build #4207

zelinh · 2023-11-10T22:57:22Z

Description

This is a milestone for the incremental build. Two methods in the run_incremental.py class are used to find whatever components have updated commits from previous build and find whatever plugins depend on them according to the input manifest.

Those additions in run_build.py is a placeholder to further implementation on incremental build.

Issues Resolved

#4201

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-11-10T23:02:09Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d39cd47) 93.17% compared to head (a93f1ca) 93.16%.
Report is 14 commits behind head on main.

Files	Patch %	Lines
src/run_build.py	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4207      +/-   ##
==========================================
- Coverage   93.17%   93.16%   -0.01%     
==========================================
  Files         188      189       +1     
  Lines        5920     5973      +53     
==========================================
+ Hits         5516     5565      +49     
- Misses        404      408       +4

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

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

gaiksaya

Taking a first pass at this PR.

Thanks @zelinh . Is this PR a 2 part? Wondering what are we doing with plugins that needs to be rebuild? The code for incremental build in run_build.py is only listing the diffs as of now. Confused as what difference is it making from build point of view.

src/build_workflow/build_args.py

src/build_workflow/build_incremental.py

gaiksaya · 2023-11-13T23:50:07Z

src/build_workflow/build_incremental.py

+    def commits_diff(self, input_manifest: InputManifest) -> List[str]:
+        build_manifest_path = os.path.join(self.distribution, "builds", input_manifest.build.filename, "manifest.yml")
+        if not os.path.exists(build_manifest_path):
+            logging.info("Previous build manifest is not exists. Rebuilding Core.")


nit

Suggested change

logging.info("Previous build manifest is not exists. Rebuilding Core.")

logging.info("Previous build manifest does not exist. Rebuilding Core.")

Also wondering why are we only re-building core? Won't that write to the directory and replace all the existing build artifacts too?

It would rebuilding all components since core changed according to the logic in below.

src/build_workflow/build_incremental.py

gaiksaya · 2023-11-14T00:21:27Z

src/run_build.py

+    if args.incremental:
+        buildIncremental = BuildIncremental(manifest, args.distribution)
+        list_of_updated_plugins = buildIncremental.commits_diff(manifest)
+        logging.info(f"Plugins for incremental build: {buildIncremental.rebuild_plugins(list_of_updated_plugins, manifest)}")
+        return 0


Pointing the exact question from the overview here: Wondering what difference is this making in overall build.

Yeah as I stated in the description of this issue, these additions in run_build.py is a placeholder to further implementation on incremental build. For this PR, it's intended for getting a list of components for rebuild.

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

rishabh6788 · 2023-11-17T19:36:35Z

src/build_workflow/build_incremental.py

+
+    # Given input manifest and return a list of what components changed and added.
+    def commits_diff(self, input_manifest: InputManifest) -> List[str]:
+        build_manifest_path = os.path.join(self.distribution, "builds", input_manifest.build.filename, "manifest.yml")


I am assuming we will be downloading the build-manifest and storing it in the path mentioned before this logic executes?

In the scope of the workflow on python side, we would assume we already have the builds folder along with build-manifest in local directory. We would need to download this entire builds folder on the CI level when we are running on Jenkins.

rishabh6788 · 2023-11-17T19:40:44Z

src/build_workflow/build_incremental.py

+            return [input_manifest.build.name.replace(" ", "-")]
+        previous_build_manifest = BuildManifest.from_path(build_manifest_path)
+        stable_input_manifest = input_manifest.stable()
+        if previous_build_manifest.build.version != stable_input_manifest.build.version:


Why this check is here?
Could there be a case where OS version in the previous build manifest differ from that in input manifest?
If yes, can you please give an example.

This came out of my mind when I was testing on local. When we are building artifacts in local, we don't have any sub-folder for different version. All built artifacts stored in the same directory following this pattern: <architecture>/builds/<opensearch/opensearch-dashboards>/<plugins/... or manifest.yml>. Even we built input manifests of different versions. Latter build-manifest will overwrite the previous one but all artifacts would all be in place without any of removal.

So I believe at least we need to check we are incrementally building the same version artifacts.

rishabh6788 · 2023-11-17T20:06:27Z

src/build_workflow/build_incremental.py

+
+    # Given updated plugins and look into the depends_on of all components to finalize a list of rebuilding components.
+    def rebuild_plugins(self, changed_plugins: List, input_manifest: InputManifest) -> List[str]:
+        if not changed_plugins:


May be add this check in the run_build.py and call this method only if you have plugins to rebuild as the method name suggests.

Yes sure. This would be considered in the current implementation on the building process for incremental build.

src/build_workflow/build_incremental.py

rishabh6788 · 2023-11-17T20:16:04Z

src/build_workflow/build_incremental.py

+        queue = changed_plugins
+        rebuild_list = []
+
+        rebuild_list.append("OpenSearch-Dashboards") if input_manifest.build.filename == "opensearch-dashboards" else rebuild_list


Did not understand this.
Aren't we supposed to rebuild everything if core is present in the changed_plugins list at line 49?

Or for OSD plugins to rebuild, we need OSD to be rebuilt again?

Yes. OSD doesn't have any those built-tool available just like OS. Every time we rebuild any of dashboards plugin, we have to build OSD core.

rishabh6788 · 2023-11-17T20:22:47Z

src/build_workflow/build_incremental.py

+        rebuild_list.append("OpenSearch-Dashboards") if input_manifest.build.filename == "opensearch-dashboards" else rebuild_list
+        while queue:
+            plugin = queue.pop(0)
+            rebuild_list.append(plugin) if plugin not in rebuild_list else rebuild_list


Why this?
Is there a possibility to have duplicates in the passed changed_plugins list?
If yes, shouldn't that be handled before it is being passed here.

This is to make sure there is no duplicate in the rebuild_list which we would return.

src/build_workflow/build_incremental.py

src/manifests/input_manifest.py

rishabh6788 · 2023-11-17T21:45:40Z

src/run_build.py

@@ -41,6 +42,12 @@ def main() -> int:

    output_dir = BuildOutputDir(manifest.build.filename, args.distribution).dir

+    if args.incremental:


please add test.

This is a placeholder for the next PR and it's just logging a list of plugins for rebuilding. I have complete tests for all those functions within buildIncremental class.

It still is part of the code flow and should be tested, if not in this PR then in subsequent PRs.

Will absolutely add tests in my following PR.

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

zelinh added 3 commits October 26, 2023 15:12

Add depends_on to input manifest

53ec862

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

Implement build incremental to get rebuilding plugins

18e0761

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

Rebase from main

5465959

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 November 10, 2023 22:57

github-actions bot added the distinguished-contributor label Nov 10, 2023

Fix groovy tests

3ac3caf

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

zelinh self-assigned this Nov 13, 2023

gaiksaya reviewed Nov 14, 2023

View reviewed changes

Update logging info

eaec41a

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

rishabh6788 reviewed Nov 17, 2023

View reviewed changes

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

rishabh6788 reviewed Nov 17, 2023

View reviewed changes

src/build_workflow/build_incremental.py Show resolved Hide resolved

rishabh6788 reviewed Nov 17, 2023

View reviewed changes

src/manifests/input_manifest.py Show resolved Hide resolved

rishabh6788 reviewed Nov 17, 2023

View reviewed changes

Add more tests

a93f1ca

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

rishabh6788 approved these changes Nov 21, 2023

View reviewed changes

zelinh merged commit dd7e905 into opensearch-project:main Nov 21, 2023
15 checks passed

zelinh deleted the commit_diff branch November 21, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commits differentiation logic for incremental build #4207

Add commits differentiation logic for incremental build #4207

zelinh commented Nov 10, 2023

codecov bot commented Nov 10, 2023 •

edited

Loading

gaiksaya left a comment

gaiksaya Nov 13, 2023

zelinh Nov 14, 2023

gaiksaya Nov 14, 2023

zelinh Nov 14, 2023

rishabh6788 Nov 17, 2023

zelinh Nov 17, 2023

rishabh6788 Nov 17, 2023 •

edited

Loading

zelinh Nov 18, 2023

rishabh6788 Nov 17, 2023

zelinh Nov 18, 2023

rishabh6788 Nov 17, 2023

rishabh6788 Nov 17, 2023

zelinh Nov 18, 2023

rishabh6788 Nov 17, 2023

zelinh Nov 18, 2023

rishabh6788 Nov 17, 2023

zelinh Nov 18, 2023

rishabh6788 Nov 18, 2023

zelinh Nov 20, 2023

	logging.info("Previous build manifest is not exists. Rebuilding Core.")
	logging.info("Previous build manifest does not exist. Rebuilding Core.")

		@@ -41,6 +42,12 @@ def main() -> int:

		output_dir = BuildOutputDir(manifest.build.filename, args.distribution).dir

		if args.incremental:

Add commits differentiation logic for incremental build #4207

Add commits differentiation logic for incremental build #4207

Conversation

zelinh commented Nov 10, 2023

Description

Issues Resolved

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

gaiksaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh6788 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 10, 2023 •

edited

Loading

rishabh6788 Nov 17, 2023 •

edited

Loading