Guidelines on breaking changes to EPP plugin parameters

[liu-cong]

EPP plugins have an opaque parameters field as a raw json string.

While this allows maximum flexibility in changes to the plugin configs, it brings the risk of easily allowing breaking changes (renaming or removal of a parameter).

I suggest the following to seed the discussion:

• Optional params:
  • Impact: does not cause EPP startup error, nor request handling error. but may lead to unexpected behavior if users fail to configure it properly due to breaking change.
  • Permission: allowed between EPP minor releases.
  • Backwards compatibility: need to support the old parameter for at least 1 more minor release after the intention removal or rename is introduced. At EPP startup we should log a WARNING: flag xx is deprecated and can be removed in the next minor release.
• Required params
  • Impact: missing of this parameter can cause EPP to crash.
  • Permission: allowed between EPP major releases.
  • Backwards compatibility: the old parameter can only be fully removed in the next major release. Also log WARNING: flag xx is deprecated and can be removed in the next major release.

Additionally we can introduce some process to properly label these changes and highlight them in release notes.

cc @nirrozenbaum @kfswain as I brought this up in a couple occasions

[ahg-g]

I suggested in the past making the parameter struct of upstream plugins versioned, just like the config api itself. This is what we did for kube-scheduler.

[liu-cong]

I suggested in the past making the parameter struct of upstream plugins versioned, just like the config api itself. This is what we did for kube-scheduler.

@ahg-g Is the parameter struct in kube-scheduler an arbitrary json or more structured?

[ahg-g]

For each plugin, it is a struct.

[nirrozenbaum]

as far as I remember, for every parameter we use in any plugins we have default value.
this means in practice that all params are optional (if not specified, default will be used).
I also don't expect users to know how to configure a plugin, except for those who were deeply involved in writing that plugin. therefore, my base assumption is that most of the time default value will be used.

I don't think versioned parameters solves anything in this case and it will over complicate things.

I support Backwards compatibility if we think that is needed.
having said that, plugins configuration change can be solved by modifying and value in a config file, without touching code.
so that is solvable in a straight forward way and I'm not quite sure anyone will even notice the deprecation warning.
personally, I'd leave things as is as long as IGW as a whole isn't GAed (only InferencePool CRD is GA).

these small changes are just the kind of things we expect when a project is in its early stages.
IGW is not GA as a whole and IMO we shouldn't treat it that way.

[JaredTan95]

personally, I'd leave things as is as long as IGW as a whole isn't GAed (only InferencePool CRD is GA).

Although I mentioned the breaking changes targeting and caring about users, as @nirrozenbaum 's thought, IGW has not been GA. We should devote more energy to making it GA instead of considering compatibility issues in advance to relieve the team's pressure at once. This will more or less affect the project progress.

However, I think we can have some discussions on this issue and leave some traces for reference when considering compatibility in the future~

[JaredTan95]

Perhaps we can adopt a compromise solution. By introducing the changlog mechanism and checking in the CI workflow, it is required that each PR must provide a changlog.yaml file based on the template, and it is necessary to specify in the file which part has been modified. And the most important thing is to self-check whether any changes will cause breaking changes to users.

When releasing the version, we can also highlight and display it according to the changlog file classification

[liu-cong]

this means in practice that all params are optional (if not specified, default will be used).

This is good in one way but it can also lead to surprising results. A user may rely on a deprecated config and expect the same behavior with a new EPP version. That's the part I am most worried about most since there is no good mechanism of signaling a misconfiguration today. And to debug one has to dig into the code history. Some warning logs help debugging significantly.

I also don't expect users to know how to configure a plugin, except for those who were deeply involved in writing that plugin.

I don't think this is 100% true. While many users will just take default values, some will customize the configs (that's why we expose them).

having said that, plugins configuration change can be solved by modifying and value in a config file, without touching code.
so that is solvable in a straight forward way and I'm not quite sure anyone will even notice the deprecation warning.

So far I have been thinking of these configs similar to flags. I have seen flag deprecation warning logs in binaries like vllm, which are quite useful and give people some room to upgrade their flags instead of a hard crash. IMO we should try to give users as much info as possible in the form of release notes, logs, etc., unless we come up with a strict versioning story.