Update remote_settings script for the new directory structure #239
Conversation
isaac-briandt
force-pushed
the
new-directory-structure
branch
from
83a65e1 to
4c1f78c
Compare
There was a problem hiding this comment.
All this looks pretty good! I'm only marking "request changes" since you are a new contributor to the project, and this is my first time reviewing your code. Otherwise I would just mark approve with my changes handled. I'm not sure if you have merge access or not, and it looks like I don't have admin rights to this repo.
I left a few minor suggestions. I did have one piece of design feedback, which I know that Erik specified this work. You can either handle this design suggestion here, or follow-up/discuss with Erik.
I'm not a fan of the term kind here, as it's fairly broad. I think a more accurate term would be architecture, as it's the model architecture that the terms base-memory and tiny refer to here.
| directory = os.path.join(RemoteSettingsClient._base_dir(args), "dev") | ||
| else: | ||
| directory = os.path.join(RemoteSettingsClient._base_dir(args), "prod") | ||
| kinds = {"base", "base-memory", "tiny"} |
There was a problem hiding this comment.
You might talk to Erik about this, but I'm not sure we should assert that the kind is part of this list. We may add new architectures at any point. Anytime we introduce a model architecture we'll have to touch this list.
| else: | ||
| directory = os.path.join(RemoteSettingsClient._base_dir(args), "prod") | ||
| kinds = {"base", "base-memory", "tiny"} | ||
| if args.kind not in kinds: |
There was a problem hiding this comment.
This could be simplified and be a bit more idiomatic:
assert args.kind in kinds, f"Invalid kind: {args.kind}. Must be one of {', '.join(kinds)}"|
|
||
| directory = os.path.join(RemoteSettingsClient._base_dir(args), args.kind) |
There was a problem hiding this comment.
_base_dir has an underscore which suggests it's a private method, but it's used as a public method. I would suggest renaming it to base_dir if you use it publicly like this.
| create_parser.add_argument( | ||
| "--kind", | ||
| type=str, | ||
| choices=["base", "base-memory", "tiny"], |
There was a problem hiding this comment.
If we're going to have a list of available model architectures, we should probably at least have a single location that we maintain it, so that if we need to update it, we only do it in once place.
Disassociated Version from Path
Passed in a --kind flag which takes in tiny, base-memory, or base
Updated the tests to align with the new directory structure and also expanded the create subcommand tests to test all of these directories.