refactor!: Make Actor initialization stricter and more predictable
#576
Conversation
github-actions
bot
added
t-tooling
…nforce explicit configuration adn services
Pijukatel
force-pushed
the
test-new-storage-services-creation-2
branch
from
9a3543f to
8bd59fd
Compare
Pijukatel
changed the title
Actor initialization stricter and more predictable
| await proxy_configuration.initialize() | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert len(caplog.records) == 2 |
There was a problem hiding this comment.
Soo what was added here? 😁
There was a problem hiding this comment.
It will throw this warning:
"'No configuration set, implicitly creating and using default Configuration.'"
Which is fine. Normally, the proxy configuration would be used within the context of the Actor and it would set the configuration.
It still works even this way, it just warns that no one set the configuration before and thus it is doing it now implicitly.
| config = Configuration.get_global_configuration() | ||
|
|
||
| async with ApifyEventManager(config): | ||
| async with ApifyEventManager(Configuration()): |
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
Not necessary, but I wanted to avoid Configuration.get_global_configuration() as it warns that it is setting the implicit default configuration as no configuration was set explicitly.
I can also properly set the configuration before the test or just pass the configuration directly and avoid the service_locator related side effects
| name: str | None = None, | ||
| alias: str | None = None, | ||
| configuration: Configuration | None = None, | ||
| configuration: CrawleeConfiguration | None = None, |
There was a problem hiding this comment.
I guess I might just be out of the loop, but wasn't this supposed to work with some "stored" configuration so that we could just remove this parameter? Is there some reason why we need it?
There was a problem hiding this comment.
I tried that approach initially, but that led to more problems related to the order in which services can be set in the service locator. Since one service is dependent on another service, but they can be set independently...
So imagine you do:
service_locator.set_configuration(Configuration1)
service_locator.set_storage_client(SomeStorageClient(Configuration2))
What now? Better to prevent this from even happening
| return cls.from_configuration(global_configuration) | ||
|
|
||
| @classmethod | ||
| def from_configuration(cls, configuration: CrawleeConfiguration) -> Configuration: |
There was a problem hiding this comment.
The result should be cached so that two calls to get_global_configuration always return the exact same object.
There was a problem hiding this comment.
I added a warning to the only potential path where this could cause a problem. If someone gets there, it is not by intention, but for the sake of backward compatibility, it will still work in most cases as expected.
src/apify/_actor.py
Outdated
|
|
||
| @cached_property | ||
| def charging_manager_implementation(self) -> ChargingManagerImplementation: | ||
| return ChargingManagerImplementation(self.config, self.apify_client) |
There was a problem hiding this comment.
I believe this should call _raise_if_not_initialized too
There was a problem hiding this comment.
src/apify/_actor.py
Outdated
| """Retrieve the charging manager to access granular pricing information.""" | ||
| self._raise_if_not_initialized() | ||
| return self._charging_manager | ||
| return self.charging_manager_implementation |
There was a problem hiding this comment.
Do we really need this method now?
There was a problem hiding this comment.
Public usage of charging_manager should be allowed only if Actor is initialized and Actor is not initialized before await self._charging_manager_implementation.__aenter__()
Co-authored-by: Vlada Dusek <[email protected]>
Description
Actorare initialized inasync init,not in__init__.Actoris considered finalized afterActor.initwas run. This also means that the same configuration used by theActoris set in the globalservice_locator.service_locatorbefore theActor.initservice_locatorand set it throughActor.(configuration=...)and runningActor.init()service_locatorand no configuration passed toActorwill create and set implicit default configurationApifyFileSystemStorageClientas local client to support pre-existing input file.ApifyStorageClientbased ontokenandapi_public_urland update NDU storage handling.Issues
Rated to: #513, #590
Testing