Add --python-dx flag to truss init
#1367
Conversation
nnarayen
force-pushed
the
nikhil/python-dx-truss-init
branch
from
69fe726 to
0864e67
Compare
|
|
||
| return target_directory_path_typed | ||
|
|
||
|
|
||
| def init_directory( |
There was a problem hiding this comment.
As always a little refactor crept into my PR 😄 . Currently, the init method below is used in exactly one non-test location, and it doesn't have the data_files, requirements_file, or bundled_packages arguments. I believe we should eventually deprecate that method, as long as we find a good way to preserve the same test coverage for all functionality that's actually used. However, I think it's cleaner to do in a separate PR.
The main reason I needed to change this is the existing init method returns a TrussHandle, which would complain with the new generated code since it doesn't have a config.yaml. The cli entrypoint actually ignores the return value (only used in tests), so I refactored into an init_directory which doesn't wrap in a TrussHandle.
There was a problem hiding this comment.
Can this fucntion be merged with _populate_target_directory?
There was a problem hiding this comment.
Or a better structuring would be to put the two if/else blocks in _populate_target_directory each into a dedicated helper function and conditionally call these in init. Also can init be module-private?
There was a problem hiding this comment.
Moved to dedicated helpers! init will go away once I port the tests to use init_directory with the same coverage, but the new entrypoint cannot be package private since cli depends on it
truss/truss_handle/build.py
Outdated
| target_directory_path: Optional[str] = None, | ||
| template: str = "custom", | ||
| populate_dirs: bool = True, | ||
| config: TrussConfig, target_directory: str, python_dx: bool = False |
There was a problem hiding this comment.
Similar to a comment below, this function is used in exactly one location and never has alternative parameters for template or populate_dirs. I removed those arguments, and instead passed in python_dx which will be derived from the CLI.
truss/truss_handle/build.py
Outdated
| else: | ||
| target_directory_path_typed = Path(target_directory_path) | ||
| target_directory_path_typed.mkdir(parents=True, exist_ok=True) | ||
| target_directory_path = Path(target_directory) |
There was a problem hiding this comment.
target_directory will always exist, so we no longer need this branch
truss/truss_handle/build.py
Outdated
| # Write config | ||
| with (target_directory_path_typed / CONFIG_FILE).open("w") as config_file: | ||
| yaml.dump(config.to_dict(verbose=False), config_file) | ||
| # Hack: We want to place the user provided model name into generated code. Until |
There was a problem hiding this comment.
I really don't love this, but didn't want to overengineer a way to get the user provided model name into the generated file
|
|
||
| class Model(baseten.ModelBase): | ||
| # Configure resources for your model here. | ||
| remote_config: baseten.RemoteConfig = baseten.RemoteConfig(name="{{ MODEL_NAME }}") |
There was a problem hiding this comment.
This template string is replaced at init time, very open to other suggestions here
| See https://truss.baseten.co/quickstart for more. | ||
| """ | ||
|
|
||
| import truss_chains as baseten |
There was a problem hiding this comment.
I think this warrants a bigger discussion: how to consolidate (brand/name-wise) chains, truss and baseten.
Without having a clear vision and general buy-in, it seems better to just stick to the established pattern of as chains for now.
There was a problem hiding this comment.
From this discussion, are you ok if we move forward with baseten for now? It's not a one way door, and I do agree that there's value in branding this differently than chains (even if that specific branding changes over time)
|
|
||
| return target_directory_path_typed | ||
|
|
||
|
|
||
| def init_directory( |
There was a problem hiding this comment.
Can this fucntion be merged with _populate_target_directory?
truss/truss_handle/build.py
Outdated
| ) | ||
|
|
||
| scaf = TrussHandle(target_directory_path) | ||
| scaf = TrussHandle(target_path) |
There was a problem hiding this comment.
I think in other places this is called th (a bit short, but it seems a widespread convention and ok in that case) - mind adjusting it?
|
|
||
| return target_directory_path_typed | ||
|
|
||
|
|
||
| def init_directory( |
There was a problem hiding this comment.
Or a better structuring would be to put the two if/else blocks in _populate_target_directory each into a dedicated helper function and conditionally call these in init. Also can init be module-private?
|
are there other places we call this "python dx"? I know we're using that phrase internally but I think it's a little confusing externally. The current DX we're using is still Python-based. What about something like |
Don't feel too strongly here, happy to move to |
🚀 What
This PR introduces an optional
--python-dxflag totruss initwhich we believe will help streamline potential adoption of the new functionality.💻 How
🔬 Testing
truss init --no-python-dxandtruss init --python dx