search

vintasoftware / django-ai-assistant

Integrate AI Assistants with Django to build intelligent applications
https://vintasoftware.github.io/django-ai-assistant/
MIT License
204 stars 12 forks source link

Drop `@register_assistant` decorator #98

Closed pamella closed 2 months ago

pamella commented 2 months ago

Issues

Resolves #97

Description

Drop @register_assistant decorator

Screencast from 19-06-2024 11:18:39.webm

Screencast from 19-06-2024 11:16:55.webm

Discussion notes

[!NOTE] Hey there, I opened this draft to get your thoughts on the new approach.

[Click to see] Resolved discussion notes > 1. We could create a `get_assistant_cls` at `django_ai_assistant/helpers/assistants.py`, but we would need to use an alias when importing it in `django_ai_assistant/helpers/use_cases.py` or renaming the existing `get_assistant_cls` > > ```python > def get_assistant_cls(assistant_id: str) -> dict[str, type[AIAssistant]]: > return AIAssistant._get_assistant_cls_registry()[assistant_id] > ``` > We opted for the alias. > 2. Without the decorator, the [`test_list_assistants_with_results`](https://github.com/vintasoftware/django-ai-assistant/actions/runs/9581319517/job/26417833376) is currently failing because it returns all `AIAssistant` classes created within `tests/`. Could it be an issue? > > ![image](https://github.com/vintasoftware/django-ai-assistant/assets/22326737/829d2143-f2d0-4738-a3cc-802776771012) We used `__init_subclass__` in `AIAssistant`, and `@pytest.fixture(scope="module", autouse=True)` in the test files.
fjsj commented 2 months ago

Overall LGTM.

We could create a get_assistant_cls at django_ai_assistant/helpers/assistants.py, but we would need to use an alias when importing it in django_ai_assistant/helpers/use_cases.py or renaming the existing get_assistant_cls

Alias is fine on the use_cases import.

Without the decorator, the test_list_assistants_with_results is currently failing because it returns all AIAssistant classes created within tests/. Could it be an issue?

That's bad because it makes unit test harder... is the problem because the __subclasses__ call isn't updated? Perhaps if we use __init_subclass__ we can solve that? See: https://stackoverflow.com/a/53101259

pamella commented 2 months ago

Alias is fine on the use_cases import.

All right, I'll go for it.

That's bad because it makes unit test harder... is the problem because the __subclasses__ call isn't updated? Perhaps if we use __init_subclass__ we can solve that? See: https://stackoverflow.com/a/53101259

I can refactor the code to use __init_subclass__, but it doesn't solve the unit testing problem. I'm checking alternatives.

pamella commented 2 months ago

I can refactor the code to use __init_subclass__, but it doesn't solve the unit testing problem. I'm checking alternatives.

I added the __init_subclass__ to the AIAssistant, and @pytest.fixture(scope="module", autouse=True) in the test files. https://docs.pytest.org/en/7.1.x/how-to/fixtures.html?highlight=scope#fixture-scopes image