Improve compare tool by vinothk-master · Pull Request #440 · mercedes-benz/odxtools

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

+        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.

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

-        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

+            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.

Hi @kakoeh,

I have made the changes as mentioned above and included a new function called print_change_metrics to print the table, especially for the services added/changed/deleted.

Thanks
Vinoth

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

            if dl.short_name in task.diagnostic_layer_names
        ]
-
+        #  print("NAMES: ",task.diagnostic_layer_names)


Hi @andlaus
Removed the dead code.
Thanks
Vinoth

Improve compare tool

vinothk-master

Hi Team

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

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

+            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

+        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

+    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.

Hi @kakoeh,

Thanks for the input, and I have made the changes as per the requirement.

Thanks
Vinoth

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

+        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"]))

Hi @kakoeh
Thanks for the input and I have made the changes as per the requirement.

Thanks
Vinoth

Sign in to view

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


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

+                    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:

Sign in to view

+                    "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)
+                })


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

+            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.

Hi @kakoeh
I haven't done the json implementation in this merge request. I felt that once the table output is printed as per the requirement we will move to this Json implementation. Could you please let me know will this work ?

Thanks
Vinoth Kannan

Hi @vinothk-master
Sure, that's a good idea

Sign in to view

+
+            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

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

…ter/odxtools

merged changes

…ter/odxtools into improve_compare_tool

kakoeh

Apart from the points discussed below, the code needs some improvements. The current implementation yields

but

python -m odxtools compare .\examples\somersault.pdx -db .\examples\somersault_modified.pdx

shows that changes in diagnostic services appear in other diagnostic layers too.
According to the output of the database comparison and the discussion in #252, I would have expected an output such as

kakoeh · 2025-09-15T07:33:57Z

+    table.add_column(
+        "Services Changed", justify="center", style="yellow", no_wrap=False, max_width=10)
+    table.add_column(
+        "Services Deleted", justify="center", style="yellow", no_wrap=False, max_width=10)


Since #442, each data object is associated with a separate color, e.g.:

rich table headers: bold cyan

file name: orange1

diagnostic layer name: green3

diagnostic layer type: medium_spring_green

numbers: yellow

It would be great to be consistent to this formatting here too.

kakoeh · 2025-09-15T08:10:08Z


-    else:
-        # no databases & no variants specified
-        rich_print("Please specify either a database or variant for a comparison")


Still, no information is printed if neither -db, -v nor -f were specified. To catch all combinations of optional arguments and reject superfluous ones, I suggest following structure of the if else block:

if args.database: if args.folder: rich_print("Both options '-db' and '-f' were specified. Please select one of these options.") if args.variants: # filter considered diagnostic layers (task.diagnostic_layer_names) # compare specified databases elif args.folder: if args.variants: # filter considered diagnostic layers (task.diagnostic_layer_names) # compare pdx files in folder elif args.variants: # no databases or folder specified # -> comparison of diagnostic layers in args.pdx_file else: # no databases, variants or folder specified rich_print("Please specify either a database (-db DATABASE [DATABASE ...]), variant (-v VARIANT [VARIANT ...]) or folder (-f FOLDER) for a comparison")

kakoeh · 2025-09-15T08:17:45Z

-        # no databases & no variants specified
-        rich_print("Please specify either a database or variant for a comparison")
+    elif getattr(args, "folder", None):
+        pdx_files = []


Still, args.pdx_file is not included in the comparison. Since this argument is mandatory, it does not make sense to skip this file, from my point of view.

kakoeh · 2025-09-15T08:52:46Z

+            summary_results: list[dict[str, int | str | None]] = []
+            file_a = pdx_files[pdx]
+            file_b = pdx_files[pdx + 1]
+            db_changes = task.compare_databases([load_file(file_a)][0], [load_file(file_b)][0])


Why the use of lists and indices?

Suggested change

db_changes = task.compare_databases([load_file(file_a)][0], [load_file(file_b)][0])

db_changes = task.compare_databases(load_file(file_a), load_file(file_b))

kakoeh · 2025-09-15T09:17:27Z

+            file_a = pdx_files[pdx]
+            file_b = pdx_files[pdx + 1]
+            db_changes = task.compare_databases([load_file(file_a)][0], [load_file(file_b)][0])
+


Suggested change

if db_changes is None:

continue

Performance can be enhanced when no changes are detected if the output of db_changes is checked. Moreover, the properties of db_changes can be annotated directly (e.g. db_changes.deleted_diagnostic_layers) as the function compare_databases returns either an object of type SpecsChangesVariants or None.

kakoeh · 2025-09-15T09:27:12Z

+
+            for layer in getattr(db_changes, "new_diagnostic_layers", []):
+                summary["new_layers"].append({
+                    "short_name": getattr(layer, "short_name", None),


short_name is a property of objects of type DiagLayer. Therefore lines 1026-1029 could be summarized to:

for layer in db_changes.new_diagnostic_layers: summary["new_layers"].append(layer.short_name)

kakoeh · 2025-09-15T09:31:17Z

+                    "services": [svc.short_name for svc in getattr(layer, "diag_comms_raw", [])],
+                }
+                summary["deleted_layers"].append(deleted_info)
+            summary["service_changes"] = getattr(db_changes, "service_changes", {})


db_changes has no attribute service_changes but changed_diagnostic_layers

kakoeh · 2025-09-15T09:43:44Z

+                "Services Deleted":
+                    len(summary["deleted_layers"][0]["services"]
+                        if summary["deleted_layers"] else []),
+            })


It seems as if something has gotten mixed up here. The number of added / deleted services is not the same as the number of added / deleted diagnostic layers as services and diagnostic layers are different types of data objects. Please revise that section again.

andlaus · 2026-03-06T12:11:23Z

@vinothk-master: do you intend to work on this in the not too distant future (address @kakoeh's comments) or should this PR be closed?

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

andlaus reviewed Aug 13, 2025

View reviewed changes

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

implementing the above changes

6df77e8

Merge branch 'main' into improve_compare_tool

4c91a17

Merge pull request #5 from vinothk-master/improve_compare_tool

d18b9e5

Improve compare tool

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

Merged

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

Merge branch 'mercedes-benz:main' into improve_compare_tool

96fd6f1

Merge branch 'mercedes-benz:main' into main

9bf55fd

Merge branch 'improve_compare_tool' of https://github.com/vinothk-mas…

65a3a0f

…ter/odxtools

save work

252a385

save work before rebase

4b9fe24

vinothk-master force-pushed the improve_compare_tool branch from f3a7c59 to 4b9fe24 Compare September 2, 2025 20:53

vinothk-master and others added 10 commits September 2, 2025 21:54

merged changes

71ca9dd

Merge pull request #7 from vinothk-master/new_refractored_codebase

cd58b64

merged changes

Merge branch 'main' into improve_compare_tool

78f9574

Linting the codebase

34ba751

Merge branch 'improve_compare_tool' of https://github.com/vinothk-mas…

e8794bf

…ter/odxtools into improve_compare_tool

Merge branch 'improve_compare_tool'

b586708

Removed copy.py

683d62b

removing pdx_files_folder

21b9550

Apply yapf formatting

2127790

Linting the files

a197504

vinothk-master requested a review from kakoeh September 9, 2025 22:49

kakoeh suggested changes Sep 15, 2025

View reviewed changes

	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:

	db_changes = task.compare_databases([load_file(file_a)][0], [load_file(file_b)][0])
	db_changes = task.compare_databases(load_file(file_a), load_file(file_b))

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!

vinothk-master Sep 3, 2025 • 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!

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!

vinothk-master Sep 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

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!

This comment was marked as outdated.

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!

vinothk-master commented Aug 13, 2025 •

edited

Loading

vinothk-master Sep 3, 2025 •

edited

Loading

vinothk-master left a comment •

edited

Loading

vinothk-master Sep 3, 2025 •

edited

Loading

kakoeh left a comment •

edited

Loading