feat: separate out remaining dependencies and improve tests#789
feat: separate out remaining dependencies and improve tests#789jakelorocco wants to merge 3 commits intomainfrom
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
jakelorocco
changed the title
jakelorocco
force-pushed
the
jal/improve-dependency-isolation-tests
branch
from
dcc97f5 to
484a590
Compare
|
@frreiss, I believe you are getting pulled in just for the changes in mellea/formatters/granite/retrievers/util.py. This is a quality of life change but let me know if you see issues with it. Thanks! |
| The heavy server dependencies are only imported when ``m serve`` is actually invoked. | ||
| """ | ||
|
|
||
| import typer |
There was a problem hiding this comment.
this is now an optional dependency if the cli bundle is installed. As such I think it should have a guard/raise an error
| except ImportError as e: | ||
| raise ImportError( | ||
| "The 'm serve' command requires extra dependencies. " | ||
| 'Please install them with: pip install "mellea[server]"' |
There was a problem hiding this comment.
typer is in the new cli dependency group. So currently we'd need server and cli (unless typer is transitively found?). Maybe the dependency is needed in server group too, or this message is modified
There was a problem hiding this comment.
Ah - I think it is transitive? I wonder if it would then be clearer in the code to split into two and keep typer aligned with a cli exception
| "ollama>=0.5.1", | ||
| "requests>=2.32.3", | ||
| "typer", | ||
| "click<8.2.0", # Newer versions will cause errors with --help in typer CLIs. |
There was a problem hiding this comment.
We used to pin click - are we ok now with the resolved version?
| @@ -8,6 +8,10 @@ resolution-markers = [ | |||
| "python_full_version < '3.12'", | |||
| ] | |||
|
|
|||
| [options] | |||
There was a problem hiding this comment.
doesn't this go in pyproject.toml as
[tool.uv]
exclude-newer = "P2W" # or "2 weeks", "14 days"
But that's based off a search - I've not tested but note the explicit timestamp in the exclude (or is that how it works?)
I think the main difference is that a uv lock will not update the timestamp - it only changes when invalidated - so we'd stick on older dependencies longer than intended (which in itself is likely a security issue)
2 participants
Misc PR
Type of PR
Description
Finishes up the dependency isolation tests; splits cli into it's own group; and fixes up import errors and dependency separation.
CI doesn't run the dependency isolation tests; ensured they pass locally.
All nice import error messages function like our current ones. The m cli works this way as well:
Testing