Lazily initialize config drivers field #1062
Conversation
|
Not sure how to unit test this because of schrodinger's initialization. Inspecting the |
|
unit tests you could inspect |
|
Technically there is no |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
the alias is just for init isnt it? the actual field still exists on the class. |
collindutter
force-pushed
the
fix/lazy-config-init
branch
from
d1b2c79
to
fabe4bb
Compare
griptape/config/config.py
Outdated
| _drivers: BaseDriverConfig = field(default=None) | ||
|
|
||
| @property | ||
| def drivers(self) -> BaseDriverConfig: | ||
| """Lazily instantiates the drivers configuration to avoid client errors like missing API key.""" | ||
| if self._drivers is None: | ||
| self._drivers = OpenAiDriverConfig() | ||
| return self._drivers |
There was a problem hiding this comment.
encapsulation to the rescue
griptape/config/config.py
Outdated
| class _Config(BaseConfig): | ||
| drivers: BaseDriverConfig = field(default=Factory(lambda: OpenAiDriverConfig()), kw_only=True) | ||
| logging: LoggingConfig = field(default=Factory(lambda: LoggingConfig()), kw_only=True) | ||
| logging: LoggingConfig = field(default=Factory(lambda: LoggingConfig())) |
There was a problem hiding this comment.
Should we be proactive and do this for logging too?
There was a problem hiding this comment.
I think I'd rather avoid introducing weird hacks like this if we can avoid it. Until there's a clear reason to introduce for logging, I say we avoid -- the API won't change either way.
collindutter
force-pushed
the
fix/lazy-config-init
branch
from
fabe4bb
to
8b9e10c
Compare
|
Ok I really don't know how to test this because of this root conftest which immediately initializes the value for all tests. According to Codecov we're already covered here 🤷♂️ |
what about setting it to |
griptape/config/config.py
Outdated
| drivers: BaseDriverConfig = field(default=Factory(lambda: OpenAiDriverConfig()), kw_only=True) | ||
| logging: LoggingConfig = field(default=Factory(lambda: LoggingConfig()), kw_only=True) | ||
| logging: LoggingConfig = field(default=Factory(lambda: LoggingConfig())) | ||
| _drivers: BaseDriverConfig = field(default=None) |
There was a problem hiding this comment.
attrs follows the doctrine that there is no such thing as a private argument and strips the underscores from the name when writing the init method signature
so removing alias=drivers makes no difference, its just less obvious what the parameter name is
tests/unit/conftest.py
Outdated
| # Some tests we don't want to use the autouse fixture's MockDriverConfig | ||
| if "skip_autouse_fixture" in request.keywords: |
There was a problem hiding this comment.
Is skip_autouse_fixture a pytest thing or something we just made up? If it is a pytest thing, does it disable all autouse fixtures? If it disabled all autouse fixtures, then why would this line be needed? I'm guessing we just made this up. If so, can we make the naming more specific to the mock_config fixture?
There was a problem hiding this comment.
Updated the name to skip_mock_config
| from .logging_config import LoggingConfig | ||
| if TYPE_CHECKING: | ||
| from .base_driver_config import BaseDriverConfig | ||
| from .logging_config import LoggingConfig | ||
|
|
||
|
|
||
| @define(kw_only=True) | ||
| class BaseConfig(SerializableMixin, ABC): |
There was a problem hiding this comment.
Out of scope for this PR, but why do we have this base class? How would we a subclass other than _Config?
I only bring it up, because the reset method in here feels weird.
There was a problem hiding this comment.
Agreed that it's probably redundant here (and maybe even adds some confusion), but I think I'd prefer to keep consistent with the pattern of having a base class for any inheritance hierarchy.
Who knows what the config classes may evolve into over time.
Describe your changes
Lazily initializes the
config.driverfield so that theOpenAiChatPromptDriverdoesn't initialize itself when the user doesn't intend to use it.Issue ticket number and link
NA