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

refactor: Tighten process registry interface - #3050

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
booxter:general-process-cleanup
Feb 4, 2025
Merged

refactor: Tighten process registry interface#3050
mergify[bot] merged 1 commit into
instructlab:mainfrom
booxter:general-process-cleanup

Conversation

@booxter

@booxter booxter commented Jan 29, 2025

Copy link
Copy Markdown
Contributor

Make it operate with the callers via Process objects; control external Process and registry modifications; provide a number of consistent verbs (load/persist; add/remove; update).

This also adds some process module specific test cases that do not rely on running the complete click machinery.

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.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

@mergify mergify Bot added testing Relates to testing ci-failure PR has at least one CI failure labels Jan 29, 2025
@booxter
booxter force-pushed the general-process-cleanup branch from ea40b5e to d584136 Compare January 29, 2025 04:02
@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 Jan 29, 2025
@booxter
booxter force-pushed the general-process-cleanup branch from d584136 to d76636f Compare January 29, 2025 04:15
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 29, 2025
@booxter
booxter requested a review from cdoern January 29, 2025 04:18
@booxter
booxter force-pushed the general-process-cleanup branch 2 times, most recently from e6bb91a to 6347f57 Compare January 29, 2025 14:27
@booxter booxter linked an issue Jan 29, 2025 that may be closed by this pull request
@booxter
booxter requested a review from alinaryan January 30, 2025 17:17
Comment thread src/instructlab/process/process.py
@booxter booxter added this to the 0.24.0 milestone Jan 30, 2025
@booxter booxter self-assigned this Jan 30, 2025

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

generally, nothing stands out here that I would correct. This looks like a really good refactoring of the process code. Besides that one bug which you already caught

Comment thread src/instructlab/process/process.py Outdated
Comment thread src/instructlab/process/process.py
@booxter
booxter force-pushed the general-process-cleanup branch from 6347f57 to a2ed233 Compare January 31, 2025 17:01
@booxter
booxter requested a review from cdoern January 31, 2025 17:02
@booxter

booxter commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

Fixed the persistence bug we noticed; and replaced TODO comment about backwards compatibility with older registry state files with a comment explaining that we SHOULD retain the compatibility. Otherwise no changes. @cdoern

@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jan 31, 2025
@booxter

booxter commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

@mergify rebase

Make it operate with dedicated Process objects; control access to
internal registry and process states; provide a number of consistent
verbs (load/persist; add/remove; update).

This also adds some process module specific test cases that do not rely
on running the complete click machinery. These cover the registry and
the process class only for now.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
@mergify

mergify Bot commented Jan 31, 2025

Copy link
Copy Markdown
Contributor

rebase

✅ Branch has been successfully rebased

@booxter
booxter force-pushed the general-process-cleanup branch from a2ed233 to 4326f27 Compare January 31, 2025 18:26
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 31, 2025
@cdoern

cdoern commented Feb 3, 2025

Copy link
Copy Markdown
Contributor

@alinaryan PTAL

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Feb 3, 2025

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

Nice work! This refactor will help a lot with maintainability

@mergify
mergify Bot merged commit aaa9f23 into instructlab:main Feb 4, 2025
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants