Skip to content

Introduce version field in CLI config yaml #1749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 19, 2024
Merged

Introduce version field in CLI config yaml #1749

merged 1 commit into from
Jul 19, 2024

Conversation

alinaryan
Copy link
Member

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@alinaryan alinaryan changed the title Introduce version field in CLI config yaml WIP: Introduce version field in CLI config yaml Jul 16, 2024
@mergify mergify bot added the ci-failure PR has at least one CI failure label Jul 16, 2024
@tiran
Copy link
Contributor

tiran commented Jul 17, 2024

Related to #1725

@alimaredia
Copy link
Contributor

Dev doc related for this PR here: instructlab/dev-docs#117

@alimaredia alimaredia added this to the 0.18.0 milestone Jul 17, 2024
@RobotSail RobotSail removed this from the 0.18.0a1 milestone Jul 17, 2024
@mergify mergify bot added the testing Relates to testing label Jul 18, 2024
@cdoern
Copy link
Contributor

cdoern commented Jul 18, 2024

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

@mergify mergify bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jul 18, 2024
@tiran
Copy link
Contributor

tiran commented Jul 18, 2024

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

@alinaryan
Copy link
Member Author

resolves #1725

@alinaryan alinaryan marked this pull request as ready for review July 18, 2024 17:24
@alinaryan alinaryan changed the title WIP: Introduce version field in CLI config yaml Introduce version field in CLI config yaml Jul 18, 2024
alimaredia
alimaredia reviewed Jul 18, 2024
View reviewed changes
src/instructlab/configuration.py Outdated
@@ -389,6 +390,7 @@ class Config(BaseModel):
def get_default_config() -> Config:
"""Generates default configuration for CLI"""
return Config(
version="1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a var

@mergify mergify bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jul 18, 2024
@alinaryan alinaryan self-assigned this Jul 18, 2024
@alimaredia alimaredia mentioned this pull request Jul 18, 2024
Merged
5 tasks
@mergify mergify bot removed the ci-failure PR has at least one CI failure label Jul 18, 2024
cdoern
cdoern approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@cdoern cdoern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this, i like the simplicity. we will follow up with some backwards compat enforcing PRs

@mergify mergify bot added the one-approval PR has one approval from a maintainer label Jul 18, 2024
RobotSail
RobotSail reviewed Jul 19, 2024
View reviewed changes
src/instructlab/configuration.py
@@ -36,6 +36,7 @@

ILAB_PACKAGE_NAME = "instructlab"
CONFIG_FILENAME = "config.yaml"
CONFIG_VERSION = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to get an opinion on this from @tiran, @leseb, or @nathan-weinberg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@alinaryan alinaryan Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alimaredia has a dev doc started here instructlab/dev-docs#117
That point can be added in

@ktam3 ktam3 added this to the 0.18.0 milestone Jul 19, 2024
RobotSail
RobotSail approved these changes Jul 19, 2024
View reviewed changes
Copy link
Member

@RobotSail RobotSail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 72365ad into instructlab:main Jul 19, 2024
16 checks passed
@mergify mergify bot removed the one-approval PR has one approval from a maintainer label Jul 19, 2024
@alinaryan alinaryan mentioned this pull request Jul 19, 2024
Closed
@ktam3 ktam3 modified the milestones: 0.18.0, 0.18.0a4 Jul 22, 2024
@derekhiggins derekhiggins mentioned this pull request Jul 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alimaredia alimaredia alimaredia left review comments

@cdoern cdoern cdoern approved these changes

@nathan-weinberg nathan-weinberg nathan-weinberg left review comments

@RobotSail RobotSail RobotSail approved these changes

Assignees

@alinaryan alinaryan

Labels
testing Relates to testing
Projects
None yet
Milestone
0.18.0a4
Development

Successfully merging this pull request may close these issues.
7 participants
@alinaryan @tiran @alimaredia @cdoern @nathan-weinberg @RobotSail @ktam3