Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

Deprecate aliased commands - #2214

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
alinaryan:dep-aliased-commands
Sep 18, 2024
Merged

Deprecate aliased commands#2214
mergify[bot] merged 1 commit into
instructlab:mainfrom
alinaryan:dep-aliased-commands

Conversation

@alinaryan

@alinaryan alinaryan commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

This change removes a majority of the aliased commands, except chat,
generate, serve, and train. When a user attempts to use an alias
that was removed, it errors out. If a user types an alias that was kept,
it proceeds with the command.
Also updates the relevant test files.

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.

@mergify mergify Bot added the ci-failure PR has at least one CI failure label Sep 6, 2024
@alinaryan alinaryan self-assigned this Sep 6, 2024

@bjhargrave bjhargrave left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think a smoother approach would be to continue to recognize the aliases but just exit after telling the user the replacement command (instead of executing the replacement command). This can help retrain users by telling them the new command.

Then in a later release, we can just remove all handling of the old aliases.

@cdoern

cdoern commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

@bjhargrave for each aliased command though we have had a warning that these would be removed in 0.19. I feel like enough warning has been given that warrants deletion of the code we no longer want to maintain. Open to what others think on this though!

@alinaryan alinaryan linked an issue Sep 6, 2024 that may be closed by this pull request
@bjhargrave

bjhargrave commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

we have had a warning that these would be removed in 0.19

I think my suggestion conforms with removing them since they no longer do anything useful and just emit a message with the proper command to use. My suggested is just a baby step to removing them by nudging the users to the proper command.

I still see many people using the old command forms since they are not forced to use the new forms. (I am not even sure they read the message :-) Just removing them without a nudge to the new command is harsher than it needs to be.

@alinaryan

Copy link
Copy Markdown
Contributor Author

I think a smoother approach would be to continue to recognize the aliases but just exit after telling the user the replacement command (instead of executing the replacement command). This can help retrain users by telling them the new command.
Then in a later release, we can just remove all handling of the old aliases.

@bjhargrave We basically have been doing this except we don't exit - just warn. The issue called for removing all handling of the old aliases, but I thought adding the transitional class with the new deprecation warning was a good compromise.

@alinaryan

Copy link
Copy Markdown
Contributor Author

continue to recognize the aliases but just exit after telling the user the replacement command

@bjhargrave I could do this though! I'll see what others think as well

@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 Sep 11, 2024
@alinaryan
alinaryan force-pushed the dep-aliased-commands branch 3 times, most recently from 352070d to 5dee11f Compare September 11, 2024 21:16
@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 Sep 11, 2024
@alinaryan

Copy link
Copy Markdown
Contributor Author

Example output running ilab and then trying to run a removed command:


(venv) alina@fedora:~/dev/instruct-lab/cli/instructlab$ ilab
Usage: ilab [OPTIONS] COMMAND [ARGS]...

  CLI for interacting with InstructLab.

  If this is your first time running ilab, it's best to start with `ilab
  config init` to create the environment.

Options:
  --config PATH  Path to a configuration file.  [default:
                 /home/alina/.config/instructlab/config.yaml]
  -v, --verbose  Enable debug logging (repeat for even more verbosity)
  --version      Show the version and exit.
  --help         Show this message and exit.

Commands:
  config    Command Group for Interacting with the Config of InstructLab.
  data      Command Group for Interacting with the Data generated by...
  model     Command Group for Interacting with the Models in InstructLab.
  system    Command group for all system-related command calls
  taxonomy  Command Group for Interacting with the Taxonomy of InstructLab.

Aliases:
  chat      model chat
  generate  data generate
  serve     model serve
  train     model train
(venv) alina@fedora:~/dev/instruct-lab/cli/instructlab$ ilab evaluate
Command 'evaluate' not found. If this was an aliased command, please note that aliases have been deprecated in 0.19.0.
Please refer to the updated documentation for the new command structure.
Usage: ilab [OPTIONS] COMMAND [ARGS]...
Try 'ilab --help' for help.

Error: No such command 'evaluate'.

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test files need to be updated too:

  • tests/test_lab_config.py
  • tests/test_lab.py

Thanks!

@bjhargrave

Copy link
Copy Markdown
Contributor

Aliases:
chat model chat
generate data generate
serve model serve
train model train

I think the help output should not mention the aliases. But if the user uses an old alias, the error message should state the replacement command to nudge the user.

@alinaryan
alinaryan marked this pull request as draft September 12, 2024 16:59
@alinaryan
alinaryan marked this pull request as draft September 12, 2024 16:59
@alinaryan
alinaryan marked this pull request as draft September 12, 2024 16:59
@mergify mergify Bot added CI/CD Affects CI/CD configuration testing Relates to testing and removed ci-failure PR has at least one CI failure labels Sep 12, 2024
@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 Sep 12, 2024

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good. Thank you for doing this! if all the tests are happy and the key aliases are gone, I am happy 🙏

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Sep 16, 2024

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One last thing, could you please update the CHANGELOG to reflect that the aliases are gone? Thanks!

@mergify mergify Bot added documentation Improvements or additions to documentation ci-failure PR has at least one CI failure labels Sep 17, 2024
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Sep 17, 2024
@alinaryan

Copy link
Copy Markdown
Contributor Author

One last thing, could you please update the CHANGELOG to reflect that the aliases are gone? Thanks!

@leseb Updated the changelog, PTAL!

Comment thread CHANGELOG.md

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry one more question!

Comment thread src/instructlab/clickext.py Outdated
Comment thread src/instructlab/clickext.py Outdated
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Sep 18, 2024
@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 Sep 18, 2024
Comment thread src/instructlab/clickext.py Outdated
Comment thread src/instructlab/clickext.py Outdated
This change removes a majority of the aliased commands, except `chat`,
`generate`, `serve`, and `train`. When a user attempts to use an alias
that was removed, it errors out. If a user types an alias that was kept,
it proceeds with the command.
Also updates the relevant test files.

Signed-off-by: Alina Ryan <aliryan@redhat.com>
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Sep 18, 2024
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Sep 18, 2024
@mergify
mergify Bot merged commit cbb3cc2 into instructlab:main Sep 18, 2024
mergify Bot added a commit that referenced this pull request Oct 6, 2024
)

Updated the README file to remove mentions of aliases that [have been removed](#2214) in the latest updates. I have also fixed the formatting to reflect the current output of the `ilab` command.









**Checklist:**

- [x] **Commit Message Formatting**: Commit titles and messages follow guidelines in the
  [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary).
- [ ] [Changelog](https://github.com/instructlab/instructlab/blob/main/CHANGELOG.md) updated with breaking and/or notable changes for the next minor release.
- [x] Documentation has been updated, if necessary.
- [ ] Unit tests have been added, if necessary.
- [ ] Functional tests have been added, if necessary.
- [ ] E2E Workflow tests have been added, if necessary.



Approved-by: bjhargrave

Approved-by: nathan-weinberg
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CI/CD Affects CI/CD configuration documentation Improvements or additions to documentation testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove some deprecated aliased commands

5 participants