Skip to content
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.

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

chore: replace platformdirs with xdg-base-dirs #269

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

jaideepr97
Copy link
Member

@jaideepr97 jaideepr97 commented Sep 10, 2024

we recently replaced platformdirs with xdg-base-dirs in the instructLab CLI
(instructlab/instructlab#2202)
this is a follow up PR to make the same update in the sdg library

This has no effect on linux, but has an impact for macOS

@mergify mergify bot added the documentation Improvements or additions to documentation label Sep 10, 2024
@mergify mergify bot added the ci-failure label Sep 10, 2024
@mergify mergify bot removed the ci-failure label Sep 10, 2024
requirements.txt Outdated Show resolved Hide resolved
@bjhargrave
Copy link
Contributor

this is a follow up PR to make the same update in the sdg library

We will then need a followup PR in instructlab to raise the dependency version for instructlab-sdg to the version which includes this PR.

@jaideepr97 jaideepr97 added dependencies Pull requests that update a dependency file and removed documentation Improvements or additions to documentation labels Sep 10, 2024
@mergify mergify bot added the documentation Improvements or additions to documentation label Sep 10, 2024
Copy link
Contributor

@bbrowning bbrowning left a comment

Choose a reason for hiding this comment

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

The code looks functionally correct to me. I pulled the changes locally and manually verified that it attempts to load pipeline configs from ~/.local/share/instructlab/sdg/pipelines and /usr/local/share/instructlab/sdg/pipelines if they exist.

I would love to see us enhance this with some unit tests that verify our assumptions about where we load the configs from. However, given there's also need to have a discussion about whether the library should directly write to the filesystem at all or leave that up to the CLI calling it, this is something we can defer until that's decided upon.

@jaideepr97 jaideepr97 merged commit b9d41ee into instructlab:main Sep 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants