Improve compare tool #440

vinothk-master · 2025-08-13T18:21:41Z

Hi @kayoub5

I have made the below changes,

provide folder with multiple pdx files as argument
load all pdx files in folder, sort them alphabetically, compare databases pairwise
When multiple pdx files are analyzed, a summary of all changes might be helpful
calculate metrics (number of added services, number of changed services, number of removed services, total number of services per ecu variant, ...)
display metrics graphically

Thanks
Vinoth Kannan

…rmat-source.sh etc.

…o refractor_compare_tool

…sh file

…sh fil

…o refractor_compare_tool

New changes from parent repo

…rint_utils.py

…o improve_compare_tool

andlaus · 2025-08-13T18:38:03Z

thanks for the contribution. @kakoeh: can you review this?

andlaus

this looks promising, but it still needs a bit of work. (I have not yet looked at the "meat" of the PR and I have some doubts about the usefulness of this feature as it is implemented by this PR (any opinions @kakoeh ?))

One thing I noticed is that the example PDX files get modified even though there is absolutely no reason why they should be (github does not allow this to be commented inline in reviews. The reason why this is there is that there are timestamps in PDX files, thus mksomersaultpdx.py creates a slightly different file every time it is called...)

andlaus · 2025-08-13T18:43:54Z

odxtools/cli/compare.py

+        summary_results = []
+        for i in range(len(pdx_files) - 1):
+            file_a = pdx_files[i]
+            file_b = pdx_files[i + 1]


this does not really make sense IMO: the desired order of files to be compared pairwise might not be lexicographical. Possibly the whole pdx_files list ought to be specified as command line parameters? (this would also remove the necessity of walking over the specified directory.)

Hi @andlaus
Thanks for the suggestion!

Actually as per the comment from @kayoub5,
load all pdx files in folder, sort them alphabetically, compare databases pairwise

this is the condition, should we go with alphabetically or do we have to compare each and every file without sorting ?

Could you please provide me your input?

Thanks
Vinoth Kannan

There is still the opportunity to pass multiple PDX files in a custom order with the command line parameter -db. From my perspective, -f FOLDER would be used when many files exists and / or typing the list of databases is considered too cumbersome. But @andlaus you're right, lexicographical order might not be desired. Assuming that the files in the folder are already sorted as wanted, pdx_files.sort() could be omitted.

odxtools/cli/compare.py

andlaus · 2025-08-13T18:46:31Z

odxtools/cli/compare.py

-        rich_print("Please specify either a database or variant for a comparison")
+    #elif args.folder:
+    elif hasattr(args, "folder") and args.folder:
+        print("Now printing the pdx files in folder")


Suggested change

print("Now printing the pdx files in folder")

print(f"PDX files in folder {args.folder}: {','.join(pdx_files)}")

(you need to move this to line 743)

andlaus · 2025-08-13T18:50:49Z

odxtools/cli/compare.py

+            if args.output:
+                with open(args.output, "w") as f:
+                    json.dump(summary_results, f, indent=4)
+        print_change_metrics(summary_results)


isn't there already a function which compares two complete PDX files? why not calling that here?

Hi @andlaus

I have reused the compare_dianostic_layer() function to compare different pdx files

Suggested change

print_change_metrics(summary_results)

changes = task.compare_diagnostic_layers(layer_a, layer_b)

Thanks
Vinoth

Hi @vinothk-master,
@andlaus is right, there is already a function which compares 2 PDX files: compare_databases()
Thus, db_changes = task.compare_databases(db_a, db_b) could be used and replace the loop for name in diagnostic_layer_names where the change metrics are calculated per diagnostic layer.
Instead of printing changes for each diagnostic layer individually with print_dl_metrics([layer_a, layer_b]), task.print_database_changes(db_changes) could then be used.

andlaus · 2025-08-13T18:51:40Z

odxtools/cli/compare.py

@@ -693,7 +710,7 @@ def run(args: argparse.Namespace) -> None:
            dl for db in task.databases for dl in db.diag_layers
            if dl.short_name in task.diagnostic_layer_names
        ]
-
+        #  print("NAMES: ",task.diagnostic_layer_names)


vinothk-master

Hi Team

Remove the dead codes
Made the changes from hasattr to getattr
Made the above changes
Thanks
Vinoth

odxtools/cli/compare.py

vinothk-master · 2025-08-13T21:10:03Z

odxtools/cli/compare.py

+            if args.output:
+                with open(args.output, "w") as f:
+                    json.dump(summary_results, f, indent=4)
+        print_change_metrics(summary_results)


Hi @andlaus

I have reused the compare_dianostic_layer() function to compare different pdx files

Suggested change

print_change_metrics(summary_results)

changes = task.compare_diagnostic_layers(layer_a, layer_b)

Thanks
Vinoth

vinothk-master · 2025-08-13T21:40:53Z

odxtools/cli/compare.py

+        summary_results = []
+        for i in range(len(pdx_files) - 1):
+            file_a = pdx_files[i]
+            file_b = pdx_files[i + 1]


Hi @andlaus
Thanks for the suggestion!

Actually as per the comment from @kayoub5,
load all pdx files in folder, sort them alphabetically, compare databases pairwise

this is the condition, should we go with alphabetically or do we have to compare each and every file without sorting ?

Could you please provide me your input?

Thanks
Vinoth Kannan

kakoeh · 2025-08-18T13:30:35Z

odxtools/cli/_print_utils.py

+    table.add_column("Services Changed", justify="right", style="yellow")
+    table.add_column("Services Deleted", justify="right", style="yellow")
+    table.add_column("Changed Parameters", justify="right", style="yellow")
+    #


Variants and Diag Layer refer to the same object, therefore I wouldn't print the comparison details for each Diag Layer. See suggestion for table design here.

kakoeh · 2025-08-18T13:36:53Z

odxtools/cli/_print_utils.py

+        table.add_row(file_pair, str(m["diag_layer"]), str(m["diag_layer_type"]),
+                      str(m["num_variants_added"]), str(m["num_variants_changed"]),
+                      str(m["num_variants_deleted"]), str(m["num_new_services"]),
+                      str(m["num_deleted_services"]), str(m["num_renamed_services"]),


According to the column headers, the order is Services Added, Services Changed, Services Deleted. Moreover, both renamed services and services with parameter changes should be characterized as Services Changed from my point of view.

Suggested change

str(m["num_deleted_services"]), str(m["num_renamed_services"]),

str(m["num_renamed_services"]+m["num_changed_parameters"]),

str(m["num_deleted_services"]))

odxtools/cli/compare.py

kakoeh · 2025-08-18T14:48:51Z

odxtools/cli/compare.py

+                    except Exception:
+                        old_names = [
+                            item for sublist in changes.changed_name_of_service for item in sublist
+                        ]


The list changes.changed_name_of_service is defined as [new_names, old_names] where

new_names is of type list[DiagService] and

old_names is of type list[str].

Moreover, changed_name_of_service is a mandatory parameter of ServiceDiff, so lines 769-776 could be deleted and num_renamed calculated as num_renamed = len(changes.changed_name_of_service[0])

kakoeh · 2025-08-18T14:49:38Z

odxtools/cli/compare.py

+                    getattr(changes, "changed_parameters_of_service", []) or [])
+
+                # collect which services had parameter changes (unique)
+                for param_detail in getattr(changes, "changed_parameters_of_service", []) or []:


Suggested change

for param_detail in getattr(changes, "changed_parameters_of_service", []) or []:

for param_detail in changes.changed_parameters_of_service:

kakoeh · 2025-08-18T15:02:57Z

odxtools/cli/compare.py

+                    "num_deleted_services": len(changes.deleted_services),
+                    "num_renamed_services": len(changes.changed_name_of_service[0]),
+                    "num_changed_parameters": len(changes.changed_parameters_of_service)
+                })


I would add here only the change metrics for services as variant is synonymous with Diag Layer
Suggestion:
Add before loop for name in diagnostic_layer_names:

summary_file = { "base_file": os.path.basename(file_a), "comparison_file": os.path.basename(file_b), "num_variants_added": len(variants_added), "num_variants_changed": variants_changed_count, "num_variants_deleted": len(variants_deleted), "changed_variants": [] }

Add at the end of loop for name in diagnostic_layer_names:

summary_file.get("changed_variants").append( { "diag_layer": changes.diag_layer, "diag_layer_type": changes.diag_layer_type, "num_new_services": num_new, "num_deleted_services": num_deleted, "num_renamed_services": num_renamed, "num_changed_parameters": num_changed_params } )

Add after the loop for name in diagnostic_layer_names:

summary_results.append(summary_file)

kakoeh · 2025-08-18T15:10:12Z

odxtools/cli/compare.py

+            print(json.dumps(summary_results, indent=4))
+            if args.output:
+                with open(args.output, "w") as f:
+                    json.dump(summary_results, f, indent=4)


Idea:
If the content of SpecsChangesVariants and ServiceDiff are mapped to the dictionary summary_results in an independent function, the changes could be printed to a json file as well if arguments -db or -v are given.

kakoeh · 2025-08-18T15:21:00Z

odxtools/cli/compare.py

+
+            print("New services:", services_b - services_a)
+            print("Deleted services:", services_a - services_b)
+            print_dl_metrics([layer_a, layer_b])


Services are defined per diagnostic layer, as well as the function print_dl_metrics([layer_a, layer_b]). Lines 804-811 should therefore be part of loop for name in diagnostic_layer_names.
Moreover, for bigger PDX files with multiple services, printing of all services for each file might be overwhelming. I'd suggest to print only the changes.

Suggested change

print_dl_metrics([layer_a, layer_b])

services_a = {srv.short_name for srv in layer_a.services}

services_b = {srv.short_name for srv in layer_b.services}

print("New services:", services_b - services_a)

print("Deleted services:", services_a - services_b)

print_dl_metrics([layer_a, layer_b])

kakoeh · 2025-08-18T15:34:00Z

pdx_files_folder/somersault.pdx

I agree with @andlaus, based on the added code, there is no necessity to upload the files somersault.pdx, somersault_modified.pdx and somersault_modified_1.pdx. Maybe it's best to add *.pdx to your .gitignore file

Maybe it's best to add *.pdx to your .gitignore file

the problem with that is that .gitignore is version controlled, i.e., it is quite easy to accidentally commit such a change. that said, maybe PDX files in the odxtools source tree ought to be ignored by everyone, i.e. changes to the somersault pdx files would need to be explicitly staged using git add -f? opinions?

Sounds reasonable

andlaus · 2025-08-27T11:28:50Z

@vinothk-master: how do you intend to proceed with this PR? (if you want to continue working on it, it would IMO be great if you coordinated you efforts with @kakoeh and her work on #442 ...)

vinothk-master · 2025-08-27T11:50:14Z

@vinothk-master: how do you intend to proceed with this PR? (if you want to continue working on it, it would IMO be great if you coordinated you efforts with @kakoeh and her work on #442 ...)

Hi @andlaus, I like to work on this PR, Let me cross check with #442 and provide my input.
Thanks
Vinoth Kannan

Co-authored-by: Katja Köhler <katja.koehler@mercedes-benz.com>

vinothk-master and others added 30 commits February 22, 2025 17:19

Reduced the type: ignore comments and allocated the structured dataclass

86b1ae5

Reduced the type: ignore comments and allocated the structured dataclass

38dec1f

fixed the lint issues

824a499

fixed the issues : change the name to service_spec, executed the refo…

716acc5

…rmat-source.sh etc.

Merge branch 'main' of https://github.com/vinothk-master/odxtools int…

89f91f2

…o refractor_compare_tool

refractored the code by adding dataclass and formated using reformat.sh

22241df

Merge branch 'mercedes-benz:main' into refractor_compare_tool

4da9b80

refractored the code by adding dataclass and formated using reformat.…

c6e45d7

…sh file

refractored the code by adding dataclass and formated using reformat.…

cf1288b

…sh file

Merge branch 'refractor_compare_tool'

87de2a2

refractored the code by adding dataclass and formated using reformat.…

c32a2cf

…sh fil

refractored the code by adding dataclass and formated using reformat.sh

4d35bd9

Merge branch 'main' of https://github.com/vinothk-master/odxtools int…

7f75ca4

…o refractor_compare_tool

Refractoring the code base with dataclass

e7bfc30

adding to remote branch

161bf41

Remove odxtools/cli/.gitignore from the repository

ef7660e

Removing the obselete codes

9f1c890

Merge branch 'mercedes-benz:main' into main

0c9f2fe

updating the branch

8cd4344

removing auto-generated non-source files

3e4d1b2

Merge branch 'mercedes-benz:main' into main

28aa654

Merge branch 'main' of https://github.com/vinothk-master/odxtools int…

1485d00

…o refractor_compare_tool

removing the Any type and removing the obselete lines

22cef5b

adding the structured code base

4940cfa

refactor the compare tool to reduce the number of ype: ignore comments

2f742b5

Adding the changes

7198894

improving both the compare and printutils files

c68064b

Merge pull request #4 from mercedes-benz/main

fbf5f0e

New changes from parent repo

Adding the changes and refractored the code base on compare.py and _p…

00e1938

…rint_utils.py

Merge branch 'main' of https://github.com/vinothk-master/odxtools int…

a52a80d

…o improve_compare_tool

vinothk-master added 2 commits August 13, 2025 18:48

adding the changes to subbranch

08cd9b4

Merge branch 'main' of https://github.com/vinothk-master/odxtools

ad689f2

andlaus reviewed Aug 13, 2025

View reviewed changes

vinothk-master and others added 2 commits August 13, 2025 21:50

implementing the above changes

6df77e8

Merge branch 'main' into improve_compare_tool

4c91a17

vinothk-master commented Aug 13, 2025

View reviewed changes

kakoeh suggested changes Aug 18, 2025

View reviewed changes

kakoeh reviewed Aug 18, 2025

View reviewed changes

andlaus mentioned this pull request Aug 27, 2025

Refactor and enhance cli tools #442

Open

vinothk-master and others added 4 commits August 27, 2025 19:01

Merge branch 'mercedes-benz:main' into improve_compare_tool

96fd6f1

Update odxtools/cli/compare.py

cd3e67e

Co-authored-by: Katja Köhler <katja.koehler@mercedes-benz.com>

Update odxtools/cli/compare.py

a7e52f6

Co-authored-by: Katja Köhler <katja.koehler@mercedes-benz.com>

Update odxtools/cli/compare.py

f3a7c59

Co-authored-by: Katja Köhler <katja.koehler@mercedes-benz.com>

	print("Now printing the pdx files in folder")
	print(f"PDX files in folder {args.folder}: {','.join(pdx_files)}")

	print_change_metrics(summary_results)
	changes = task.compare_diagnostic_layers(layer_a, layer_b)

	str(m["num_deleted_services"]), str(m["num_renamed_services"]),
	str(m["num_renamed_services"]+m["num_changed_parameters"]),
	str(m["num_deleted_services"]))

	for param_detail in getattr(changes, "changed_parameters_of_service", []) or []:
	for param_detail in changes.changed_parameters_of_service:

Improve compare tool #440

Are you sure you want to change the base?

Improve compare tool #440

Uh oh!

Conversation

vinothk-master commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andlaus commented Aug 13, 2025

Uh oh!

andlaus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinothk-master left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andlaus commented Aug 27, 2025

Uh oh!

vinothk-master commented Aug 27, 2025

Uh oh!

Uh oh!

vinothk-master commented Aug 13, 2025 •

edited

Loading

vinothk-master left a comment •

edited

Loading