Use sys.executable for Python commands and add LLM config options - #165
Use sys.executable for Python commands and add LLM config options#165DomPTech wants to merge 8 commits into
Conversation
…ad of uv so they have their actual PIDs tracked
…tually used (also downloads ollama model if not available); docs for LLM device properties
…t more transport protocols
…Client instead of multiple; added `mcp_connections` attribute
There was a problem hiding this comment.
Just reading the notebook :
I wonder if below means the agent was not able to call the right tools and then failed? Will increasing Recursion limit help?
prompt = "Get a scanned haadf image. Then get an EDS spectrum."
response = llm.query(prompt)
print(response)Recursion limit of 5 reached without hitting a stop condition. You can increase the limit by setting the recursion_limit config key.
For troubleshooting, visit: https://docs.langchain.com/oss/python/langgraph/errors/GRAPH_RECURSION_LIMIT
There was a problem hiding this comment.
This was a very interesting thing during testing actually. Basically what happened was the agent successfully acquired the image, and then tried to acquire an EDS spectrum. However, for some reason the acquire_spectrum tool on the DigitalTwin always seems to fail with a Tango error (according to the stack trace because the EDS device is not properly initialized). Thus it continued to try different things to no avail, even though it technically was correct. Perhaps we should raise an issue for this.
There was a problem hiding this comment.
Interesting. A issue on this would be nice.
| chat_model_name = device_property(dtype=str, default_value="gpt-4o", doc="The name of the chat model to use for the LLM") | ||
| api_key = device_property(dtype=str, default_value="", doc="The API key for the model provider") | ||
| api_base = device_property(dtype=str, default_value="", doc="The base URL for the API") | ||
|
|
||
| # Ollama config | ||
| ollama_model = device_property(dtype=str, default_value="gemma4:31b", doc="The Ollama model ID to use for the LLM") | ||
| auto_pull_model = device_property(dtype=bool, default_value=True, doc="If true, automatically pull the Ollama model if it is not already downloaded.") |
There was a problem hiding this comment.
I wonder why ollama is treated here as a special case? just because it is offline?
There was a problem hiding this comment.
Correct (this is the provider we tend to use the most often), so I added special functionality for auto starting ollama servers, pulling models, etc. But your comment actually did make me realize we don't need a device_property of ollama_model (since you can just do provider of ollama and chat_model_name of "gemma4:31b" example).
| # Generic init_chat_model config | ||
| chat_model_name = device_property(dtype=str, default_value="gpt-4o", doc="The name of the chat model to use for the LLM") | ||
| api_key = device_property(dtype=str, default_value="", doc="The API key for the model provider") | ||
| api_base = device_property(dtype=str, default_value="", doc="The base URL for the API") |
There was a problem hiding this comment.
If chat chat_model_name has default value gpt-4o the api_base should be https://api.openai.com/v1?
There was a problem hiding this comment.
init_chat_model doesn't require an api_base for most providers (for example if you wanted to use openai you just set the provider to openai and the model name to gpt-4o and it automatically handles the routing).
|
|
||
| try: | ||
|
|
||
| if self.auto_pull_model: # Run command to download the model if it is not already |
There was a problem hiding this comment.
I would give a sort of warning message to the user with list of models already there to choose from. More like: print("this model_x is not available, these models are available .....model_a, model_b..., either choose from these or Proceed as is to pull the model_x which may take some time")
LLM Device:
LLMdevice properties to include more detailed configuration options (such aschat_model_name,api_key,api_base)auto_pull_modelpropertyStartup Scripts:
ollama_modelpropertyLLMConfigclass matches the new propertiesrun_serverandrun_llmscripts to usesys.executableinstead ofuv. Originally we would calluv run python -m <detector_process>, which would first create a uv process, and then that process would create the underlying python process. However due to the volatility of process tracking/killing, the python process could become detached from the parent uv process, or killing the uv process might not properly terminate its children. Usingsys.executableinstead (which was already done in the DATA device for the tiled command) means thatProcessManagertracks the actual python process itself, instead of indirectly through uv.