Cli improvements v0.1 #64
Conversation
resin_cli/cli.py
Outdated
| if not is_healthy(chat_service_url): | ||
| msg = ( | ||
| f"Resin service is not running! on {chat_service_url}" | ||
| + " please run `resin start`" |
There was a problem hiding this comment.
| + " please run `resin start`" | |
| " please run `resin start`" |
You don't need the + here (in fact - this is a bug! python errors out).
Please remove all +s for multiline strings inside ()
resin/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| __version__ = '0.1.0' | |||
There was a problem hiding this comment.
That's not how do set the version!!!
The version needs to be set in the pyproject.toml file, to be correctly processed by PyPI etc.
You don't need to create duplication.
There was a problem hiding this comment.
If you need to read the version in the code itself - we should find the proper manner to read it directly from the package (after package was properly installed with pip install, that should be doable).
There was a problem hiding this comment.
This is the correct solution: https://stackoverflow.com/a/67097076
tests/unit/cli/test_data_loader.py
Outdated
| @@ -78,6 +87,17 @@ | |||
| ] | |||
| ) | |||
|
|
|||
| bad_df_metadata_not_allowed_all_permutations = pd.DataFrame( | |||
There was a problem hiding this comment.
This is a bad test. It will fail for any of the permutation (the first one), so we won't check that we correctly error out for all cases
resin_cli/cli.py
Outdated
| "Error: Failed to connect to Pinecone index, please make sure" | ||
| + " you have set the right env vars" | ||
| + " PINECONE_API_KEY, INDEX_NAME, PINECONE_ENVIRONMENT" |
There was a problem hiding this comment.
This error message is too general and too broad.
We should at least try to catch different type of errors which already have (e.g. RuntimeError), and include some of the original error message.
The KnowledgeBase is already returning meaningful error messages per different failure reason - and we're just throwing that information away
resin_cli/cli.py
Outdated
| "Error: Failed to upsert data to index" | ||
| + f" {INDEX_NAME_PREFIX}{index_name}" | ||
| + " this could be due to connection issues" | ||
| + " please re-run `resin upsert`" |
There was a problem hiding this comment.
Again - this is far too general and not actionable. You should use the information in the error raised by KnowledgeBase.
For now this could even be an if on the str(e), at least until we'll have proper error types.
resin_cli/cli.py
Outdated
| @click.option("--host", default="0.0.0.0", help="Host") | ||
| @click.option("--port", default=8000, help="Port") | ||
| @click.option("--ssl/--no-ssl", default=False, help="SSL") |
There was a problem hiding this comment.
You need elaborate, explanative help message for every param of every command, not just the commands themselves
Since the package is installed, we can read the right version using `importlib
A. Changed is_healthy to simply exit, instead of being a bool - saves a lot of code duplication B. Distinguish between the case of service not running to service is running but unhealthy
Give more detailed indication of the actual underlying problem.
Made them a bit more explicit.
|
Closing the PR, and opening a new one as author (from the same branch) |
Problem
wip
Solution
wip
Type of Change
Test Plan
Describe specific steps for validating this change.