Add doc for dedicated local storage in ilab cli #104

RobotSail · 2024-06-24T21:41:10Z

This document outlines an approach for config and program data management that simplifies
the user flow and creates a streamlined experience.

Signed-off-by: Oleg S [email protected]

Signed-off-by: Oleg S <[email protected]>

russellb

I'm generally happy with the direction here.

I'd like to hear a little more around the proposed unique refs -- how would you generate them? where would they be stored? How would they be mapped to a file (or files) on disk?

Are you intentionally leaving it vague and just "TBD based on whatever seems easy to implement" later? or do you have anything more concrete in mind?

There's also some linter complaints, but I'm sure you saw those already.

russellb · 2024-06-25T00:36:23Z

docs/cli/ilab-local-config.md

+
+For tools to be useful to the user, they generally need to work on and know about data pertinent to the user. For example, podman and docker track image blobs, kubectl tracks cluster credentials, etc.
+
+The InstructLab CLI manages a number of settings today such as the teacher model, taxonomy location, inference model, etc. using a local file known as `config.yaml`.


The CLI itself doesn't manage any of this. It's user config. It generates an initial file for you, but never updates it from there. I only bring this up because user config isn't the same as data that is never touched directly by a user. (config vs data)

This is a good point, I would say it makes a stronger point for the proposed structure of the ~/.local/share directory.

That might be so @RobotSail but as @russellb mentioned the CLI doesn't manage this.

russellb · 2024-06-25T00:37:30Z

docs/cli/ilab-local-config.md

+
+We propose the following:
+
+1. `ilab` will have a dedicated config directory seated at `~/.config/ilab` on Linux/MacOS systems, and the `%APPDATA%` equivalent if we decide to support Windows.


presumably this works on windows with WSL? I know people have been using it on WSL, already.

If it's WSL then we should still assume it's a linux system. The only WSL-specific bits are more on the CUDA drivers side. And even then, that works fine currently.

I was thinking more of "raw windows", but you're right - WSL is a great alternative.

@RobotSail Are qwe going to support native Windows? if not, we should be explicit that it is WSL.

russellb · 2024-06-25T00:43:21Z

docs/cli/ilab-local-config.md

+
+The steps outlined to implement this are:
+
+1. Implement logic for `~/.config/ilab` and `~/.local/share/ilab` directories and expect the config to live and be read from there unless another config is manually specified. For simplicity, if the is read in from `~/.config/ilab` then we assume the relative paths should be realtive to the data directory `~/.local/share/ilab`.


A way this can work is have a data_dir that can be changed via a CLI flag or config file option, but always default to what you have specified here -- very similar to what you describe as a default location for the config dir unless otherwise specified via a flag

Yes I agree with that.

docs/cli/ilab-local-config.md

RobotSail · 2024-06-25T13:44:01Z

I'd like to hear a little more around the proposed unique refs -- how would you generate them? where would they be stored? How would they be mapped to a file (or files) on disk?

Are you intentionally leaving it vague and just "TBD based on whatever seems easy to implement" later? or do you have anything more concrete in mind?

I was thinking that we could just use a simple UUID or SHA2 digest. For the initial version we could have the refs just be the relative paths in the ~/.local/share directory.

Signed-off-by: Oleg S <[email protected]>

docs/cli/ilab-local-config.md

n1hility · 2024-06-25T20:46:05Z

docs/cli/ilab-local-config.md

+
+We propose the following:
+
+1. `ilab` will have a dedicated config directory seated at `~/.config/instructlab` on Linux/MacOS systems, and the `%APPDATA%` equivalent if Windows support is added in the future.


Is your intent to support XDG?:
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html#introduction

Not arguing that you should necessarily but if you intended to I would change the text to be seated at $XDG_CONFIG_HOME/instructlab (.local/share/instructlab) (And XDG_DATA_HOME later on)

Yes that's what I was thinking of.

I strongly recommend to use https://pypi.org/project/platformdirs/ . It has the correct defaults for all important platforms.

I also like to point out that the use of XDG directories does not work well with systemd services. @mairin just mentioned that she wants systemd service support for InstructLab. XDG directories are problematic, because they are designed for human end-users with a home directory. systemd --system need an entirely different directory structure.

@tiran Thanks for pointing this out! I had initially suggested XDG thinking of the laptop-targeted experience. But if we want a systemd service we'll need to use system directories. Ideally we'd use XDG for the laptop-target user experience and system-wide directories for the big mama GPU hw experience.

^ to this end, @tiran suggests I would store the paths in config.yaml and have a option to either use XDG or a common base directory for all paths.

n1hility · 2024-06-25T20:48:44Z

docs/cli/ilab-local-config.md

+The output would look something like
+
+
+## Evolving configs


This seems like a distinct proposal, was there a reason you included it?

Yes this is definitely distinct, but will be something we'll need as a feature in the future.

n1hility · 2024-06-25T20:53:57Z

docs/cli/ilab-local-config.md

+We propose the following:
+
+1. `ilab` will have a dedicated config directory seated at `~/.config/instructlab` on Linux/MacOS systems, and the `%APPDATA%` equivalent if Windows support is added in the future.
+2. `ilab` will have a dedicated program files directory seated at `~/.local/share/instructlab` which will store model checkpoints, downloaded models, etc. 


To carry through on the Windows spec I would say %LOCALAPPDATA% here.

Thanks for the clarification!

n1hility · 2024-06-25T20:56:10Z

docs/cli/ilab-local-config.md

+
+Based on these steps, #1 would be the trickiest because we assume that the paths referenced in the config file are relative to the calling process. 
+
+One approach here could be removing these paths from the default and forcing the user to specify them relatively.


I am having trouble understanding what you mean on this sentence

@n1hility My wording here is bad but the idea is simple:

When the config read is the default one ~/.config/ilab/config.yaml then parse paths relative to ~/.local/share/ilab.

When that's NOT the config, we would parse relative paths with respect to the calling process. E.g. if ilab is invoked in /foo and the config.model_path references a file in bar/granite_7b.gguf then it would resolve to /foo/bar/granite_7b.gguf.

All absolute paths will be parsed as-is

n1hility · 2024-06-25T20:57:37Z

docs/cli/ilab-local-config.md

+
+One approach here could be removing these paths from the default and forcing the user to specify them relatively.
+
+Another approach would be implementing the list commands at the same time as #1 and setting it up so that the relative paths passed to `ilab` are relative ***to the `~/.local/share/instructlab` directory***


Is there any compatibility with existing configs? For example it looks in the current dir and if there is a config.yml expects abs paths, or is this an intentional major change

What would the experience be like without #3 (implementing refs) in your list?

Would list tell you were a file was in that case?

This would be a breaking change then but we'd need to make it. We can maintain compatibility by allowing users to provide a --config flag which should read in the config as we do today. Not sure how much harder this would be versus what we're doing today.

n1hility

Overall I really like this proposal, but could use some clarifications on a few points made.

Signed-off-by: Oleg S <[email protected]>

hickeyma

I think overall this is a good proposal and direction. Thanks for pushing this @RobotSail.

Some additional points:

As raised by @n1hility, I think supporting the XDG Base Directory Specification is a good idea. Do you mind adding this to the proposal? You could then mention support for the environment variables as per the XDG Base Directory Specification: $XDG_CONFIG_HOME and $XDG_DATA_HOME.
It was mentioned about capability to change the data and config paths. This could be implemented by providing environment variables like $INSTRUCTLAB_PATH_CONFIG and $INSTRUCTLAB_PATH_CONFIG

hickeyma · 2024-07-04T11:07:58Z

docs/cli/ilab-local-config.md

+
+The steps outlined to implement this are:
+
+1. Implement logic for the `~/.config/instructlab` and `~/.local/share/instructlab` directories, and expect the config to live and be read from there unless another config is manually specified. For simplicity, if the config we read in is located at `~/.config/instructlab` - the default location - then we'll assume relative paths should be resolved with respect to the data directory `~/.local/share/instructlab`.


Should it be that relative paths are resolved with respect to the config directory?

danmcp · 2024-08-27T17:10:21Z

@RobotSail Would you be willing to update this doc based on feedback and current state? Would be good to get it merged given the state of the code has already moved past the state of this doc.

RobotSail · 2024-08-27T17:12:01Z

@danmcp Yes we can update this one to reflect what currently exists in ilab

old review, code moved on independent of this doc

danmcp · 2024-08-27T17:51:04Z

@danmcp Yes we can update this one to reflect what currently exists in ilab

Thank you!

nathan-weinberg · 2024-10-29T20:06:04Z

@RobotSail are you still planning on updating this?

add doc for dedicated local storage in ilab

d6c81b3

Signed-off-by: Oleg S <[email protected]>

RobotSail requested review from cdoern and jeremyeder June 24, 2024 23:01

russellb previously requested changes Jun 25, 2024

View reviewed changes

nathan-weinberg requested changes Jun 25, 2024

View reviewed changes

update dev doc based on recommendations

93ac391

Signed-off-by: Oleg S <[email protected]>

nathan-weinberg requested changes Jun 25, 2024

View reviewed changes

n1hility reviewed Jun 25, 2024

View reviewed changes

n1hility requested changes Jun 25, 2024

View reviewed changes

updates

2ba3e5a

Signed-off-by: Oleg S <[email protected]>

RobotSail mentioned this pull request Jun 26, 2024

Implementation of Dedicated Storage instructlab/instructlab#1471

Merged

hickeyma requested changes Jul 4, 2024

View reviewed changes

RobotSail mentioned this pull request Sep 10, 2024

Use of ~/Library/Application Support on Mac is user-unfriendly instructlab/instructlab#1747

Closed

RobotSail mentioned this pull request Nov 11, 2024

feat: evaluate can now format output as JSON instructlab/instructlab#2470

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add doc for dedicated local storage in ilab cli #104

Add doc for dedicated local storage in ilab cli #104

RobotSail commented Jun 24, 2024

russellb left a comment

russellb Jun 25, 2024

RobotSail Jun 25, 2024

hickeyma Jul 4, 2024

russellb Jun 25, 2024

RobotSail Jun 25, 2024

RobotSail Jun 25, 2024

hickeyma Jul 4, 2024

russellb Jun 25, 2024

RobotSail Jun 25, 2024

RobotSail commented Jun 25, 2024

n1hility Jun 25, 2024 •

edited

Loading

RobotSail Jun 25, 2024

tiran Jul 4, 2024

mairin Jul 4, 2024

mairin Jul 4, 2024

n1hility Jun 25, 2024

RobotSail Jun 25, 2024

n1hility Jun 25, 2024

RobotSail Jun 25, 2024

n1hility Jun 25, 2024 •

edited

Loading

RobotSail Jun 25, 2024

n1hility Jun 25, 2024

n1hility Jun 25, 2024

n1hility Jun 25, 2024

RobotSail Jun 25, 2024

n1hility left a comment

hickeyma left a comment

hickeyma Jul 4, 2024

danmcp commented Aug 27, 2024

RobotSail commented Aug 27, 2024

danmcp commented Aug 27, 2024

nathan-weinberg commented Oct 29, 2024


		For tools to be useful to the user, they generally need to work on and know about data pertinent to the user. For example, podman and docker track image blobs, kubectl tracks cluster credentials, etc.

		The InstructLab CLI manages a number of settings today such as the teacher model, taxonomy location, inference model, etc. using a local file known as `config.yaml`.


		We propose the following:

		1. `ilab` will have a dedicated config directory seated at `~/.config/ilab` on Linux/MacOS systems, and the `%APPDATA%` equivalent if we decide to support Windows.


		The steps outlined to implement this are:

		1. Implement logic for `~/.config/ilab` and `~/.local/share/ilab` directories and expect the config to live and be read from there unless another config is manually specified. For simplicity, if the is read in from `~/.config/ilab` then we assume the relative paths should be realtive to the data directory `~/.local/share/ilab`.


		Based on these steps, #1 would be the trickiest because we assume that the paths referenced in the config file are relative to the calling process.

		One approach here could be removing these paths from the default and forcing the user to specify them relatively.


		One approach here could be removing these paths from the default and forcing the user to specify them relatively.

		Another approach would be implementing the list commands at the same time as #1 and setting it up so that the relative paths passed to `ilab` are relative *to the `~/.local/share/instructlab` directory*


		The steps outlined to implement this are:

		1. Implement logic for the `~/.config/instructlab` and `~/.local/share/instructlab` directories, and expect the config to live and be read from there unless another config is manually specified. For simplicity, if the config we read in is located at `~/.config/instructlab` - the default location - then we'll assume relative paths should be resolved with respect to the data directory `~/.local/share/instructlab`.

Add doc for dedicated local storage in ilab cli #104

Are you sure you want to change the base?

Add doc for dedicated local storage in ilab cli #104

Conversation

RobotSail commented Jun 24, 2024

russellb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobotSail commented Jun 25, 2024

n1hility Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1hility Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1hility left a comment

Choose a reason for hiding this comment

hickeyma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmcp commented Aug 27, 2024

RobotSail commented Aug 27, 2024

danmcp commented Aug 27, 2024

nathan-weinberg commented Oct 29, 2024

n1hility Jun 25, 2024 •

edited

Loading

n1hility Jun 25, 2024 •

edited

Loading