Introduce version field in CLI config yaml#1749
Introduce version field in CLI config yaml#1749mergify[bot] merged 1 commit intoinstructlab:mainfrom
Conversation
alinaryan
changed the title
|
Related to #1725 |
|
Dev doc related for this PR here: instructlab/dev-docs#117 |
alinaryan
force-pushed
the
addConfigVersion
branch
from
4d1374d to
94c891a
Compare
|
spoke with @alinaryan. I think this PR (for the sake of keeping development going). Should JUST focus on adding the cfg field. me, @alimaredia, and @alinaryan will follow up with a separate PR working on a backwards compat and version enforcing mechanism. IMO these are two different tasks and make sense to be split up |
alinaryan
force-pushed
the
addConfigVersion
branch
from
94c891a to
3fe24ce
Compare
mergify
bot
added
ci-failure
|
@alimaredia @cdoern #1683 adds test and infrastructure to track config changes in git. This will help us to detect breaking changes or understand config changes in git history. |
|
resolves #1725 |
alinaryan
changed the title
src/instructlab/configuration.py
Outdated
| def get_default_config() -> Config: | ||
| """Generates default configuration for CLI""" | ||
| return Config( | ||
| version="1.0.0", |
There was a problem hiding this comment.
We're going to want to set this as a variable somewhere else in this file.
I would put it right above the variable for the config file name itself, https://github.com/instructlab/instructlab/blob/main/src/instructlab/configuration.py#L39.
alinaryan
force-pushed
the
addConfigVersion
branch
from
3fe24ce to
e05bccc
Compare
mergify
bot
added
ci-failure
Signed-off-by: Alina Ryan <aliryan@redhat.com>
alinaryan
force-pushed
the
addConfigVersion
branch
from
e05bccc to
69d5bbf
Compare
cdoern
left a comment
cdoern
left a comment
There was a problem hiding this comment.
thanks for this, i like the simplicity. we will follow up with some backwards compat enforcing PRs
|
|
||
| ILAB_PACKAGE_NAME = "instructlab" | ||
| CONFIG_FILENAME = "config.yaml" | ||
| CONFIG_VERSION = "1.0.0" |
There was a problem hiding this comment.
Should we be doing the versioning here using semver, or should we instead do a discrete number like v1, v2, etc., so that we can handle migrations between configs?
There was a problem hiding this comment.
Would like to get an opinion on this from @tiran, @leseb, or @nathan-weinberg.
There was a problem hiding this comment.
I think a discrete number may make more sense here, we do something like that for the schema: https://github.com/instructlab/schema/tree/main/src/instructlab/schema
There was a problem hiding this comment.
as long as v1 can also be v1.1 or something like that I am fine with it.
We need to have different major/minor versions of the cfg in between minor releases
There was a problem hiding this comment.
Based on some preliminary research, it seems like there are a few tradeoffs. We should create a dev-doc for the broader migration system as it seems to be a heavy lift.
I propose that we keep this as 1.0.0 instead of doing discrete numbers because of this exact point:
We need to have different major/minor versions of the cfg in between minor releases
If we decide that we want to use a scheme of discrete numbers instead, we can simply map 1.0.0 to v1.
There was a problem hiding this comment.
@alimaredia has a dev doc started here instructlab/dev-docs#117
That point can be added in
7 participants
Checklist:
conventional commits.