[AliRana30]

Summary of Changes

• Configurable ID Encoding: Replaced the hardcoded final boolean encodeIds = false; in TreeEncoder with wayang.ml.encoding.encode-ids (defaulting to false, with shorthand wayang.ml.encode-ids supported).
• TreeEncoder Enhancements: Added TreeEncoder(mappings, configuration) constructor, getter/setter for configuration, and overloaded encode methods that resolve encodeIds from OptimizationContext or local configuration.
• Default Properties: Loaded wayang-ml-defaults.properties in MachineLearning.plugin().setProperties(Configuration) and declared wayang.ml.encoding.encode-ids = false.
• Unit Tests: Added test cases in TreeEncoderTest verifying default behavior, enabled ID encoding via configuration, and plugin properties loading.

Closes #776.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[AliRana30]

@zkaoudi Needed your review here.

[novatechflow]

Thanks, two issues:

• Loading the defaults during plugin registration overwrites user configuration, including encode-ids=true, the model path, experience collection, and reoptimization settings.
• The shorthand wayang.ml.encode-ids=true is ignored after registration because the default canonical property is always present as false.

Reproduced both with focused Java 17 checks. Always preserve user settings, fix shorthand resolution, and add regression tests covering plugin registration.

[AliRana30]

Thanks for the review @novatechflow ! Both issues are fixed and tested on Java 17:

1. User settings preserved: MachineLearning.setProperties now applies a default only if the user hasn't already set it. This covers encode-ids, the model path, experience collection and reoptimization.
2. Shorthand fixed: wayang.ml.encode-ids=true is no longer shadowed by the default canonical false.
3. Tests: Added regression tests in TreeEncoderTest for plugin registration and shorthand resolution.

[novatechflow]

The original issues are fixed and covered by regression tests, great progress!

One edge remains: a job-level false cannot override an inherited true when the canonical and shorthand properties differ.

Reproduced on Java 17. Please add parent/child override tests.

[AliRana30]

@novatechflow all your requirements have been added. A job-level false now overrides an inherited true in either direction. Resolution picks the nearest configuration level that sets either spelling, and the plugin no longer injects an encode-ids default that could shadow it. Added parent/child override tests, including 3-level chains and plugin registration.

[novatechflow]

Java 17 probes show the resolver confuses explicit settings with cached inherited values:

• Parent canonical false; child explicitly sets canonical false and shorthand true => resolves to true, ignoring explicit canonical false.
• After caching an inherited value, changing the parent can make the child resolve to false even though its shorthand remains explicitly true.

The value-comparison heuristic needs reliable tracking of explicit versus cached properties.

[AliRana30]

@novatechflow Replaced the value-comparison heuristic with explicit property tracking in MapBasedKeyValueProvider:

• Explicit vs. Cached Tracking: MapBasedKeyValueProvider now tracks explicitly set keys (provideExplicitLocally), separating them from entries populated via parent delegation caching (processParentEntry).
• Resolver Semantics: TreeEncoder.resolveEncodeIds inspects provideExplicitLocally at each level (child -> parent). On the same level, an explicit canonical property takes precedence over shorthand; cached parent values on the child never masquerade as explicit overrides or shadow subsequent parent updates.
• Regression Tests: Added MapBasedKeyValueProviderTest and unit tests in TreeEncoderTest verifying the parent false + child canonical false/shorthand true resolution and parent mutations after cached child reads.

[novatechflow]

Great progress - you picked a really tricky one, btw.

One remaining part: plugin registration ignores explicit settings on Configuration.getDefaultConfiguration() when configuring a child.

Reproduce with Java 17: set the global model path to /custom/global.onnx, create a child configuration, then register the plugin, the child gets the bundled model path instead.

When reading the code I think the hierarchy scan stops before checking the global configuration. It should include that level’s explicit properties and add a regression test. Thoughts?

[AliRana30]

Spot on and thanks for catching that! The loop stopped right before inspecting Configuration.getDefaultConfiguration().

• Updated isExplicitlyConfigured in MachineLearning to scan the full ancestry (for (Configuration current = configuration; current != null; current = current.getParent())), including Configuration.getDefaultConfiguration().
• Added removeProperty to Configuration to cleanly restore global state in tests.
• Excluded binary **/*.onnx models from the apache-rat-plugin license check in root pom.xml.
• Added regression tests in TreeEncoderTest covering global explicit model paths, global encode-ids, multi-level hierarchy propagation, and child-level overrides.

@novatechflow

[novatechflow]

Java 17 checks pass: five provider tests, four new global-setting regressions, and the previous override/cache probes. Global model paths are preserved, and child overrides still work.

Thanks @AliRana30 for your contribution!