Replies: 12 comments 6 replies
-
Thanks for getting back to me with thoughtful feedback no less!
I'm not sure I follow. For me the main difference between Registries and Containers is their lifecycle. A value has the same lifetime as a Registry; so it makes sense to me to let it live in the Registry?
That makes a lot of sense; I never quite realized that recursive resources only work with Flask's threadlocal approach, because I didn't need it elsewhere until now (full disclosure: I currently only use svcs with Flask and AIOHTTP whose integration I'm close to merging).
I understand your concern, but I'm not going for purity but to get rid of common boilerplate. :) I guess there could be a more general approach that employs some kind of metadata and filtering on that, but I think I can live with the current solution for now.
Does it? You mean like a general callback system on certain lifecycle stages? I mean that sounds interesting, but I suspect it's better to watch adoption/feedback to get a better idea of what's used/needed. 🤔
It would! I'm just trying to keep the API envelope as narrow as possible for now to avoid painting myself into a corner.
That's at least currently not possible for me, because I'm iterating too fast (including the occasional
I do like it, but is it a good name for mass-adoption? 😅 I actually literally don't know; I'm not famous for great package-naming.
I wonder what it would take to make What would you consider a good mechanic for QoL? I was thinking contextvars.
Could you elaborate a bit more please? Assuming Except for the container argument, would there be anything stopping you from implementing it yourself? 🤔 ← this is generally my litmus test for good APIs: build it in a way such that people can help themselves 99% of the time Thanks again and keep the feedback coming! I'm getting increasingly excited about this project, even if it's gonna have only two users. ;) |
Beta Was this translation helpful? Give feedback.
-
JFTR I've decided to make this a discussion so we can keep it running and open actionable issues as they appear. |
Beta Was this translation helpful? Give feedback.
-
I would think that But in the end it might be just a matter of preference, both work :)
Yeah that makes sense :)
sounds reasonable
That is actually a good idea. I think most of the functionality would be easily reimplemented using svcs as a basis. I would need to rethink containers/repositories as context managers or get rid of it. I can currently do this: @pytest.fixture
def repository(): # a repository serves both as a registry and a container
with gimme.context() as repo:
yield repo this creates a layer on top of an existing repository (ie the global/QoL repository) on which I can add instances and factories that are cleaned up when the context exits scope, very useful for testing. Requesting an instance using the global api (QoL) always starts at the top of the stack. If a factory or instance doesn't exist on the current layer, it moves down the stack until it finds one. I can then use it like this @pytest.fixture(autouse=True)
def mocked_service(repository):
class FakeService(MyService):
"""some fake implementation"""
repository.add(FakeService()) any test that that uses code that requests a I'd need to think about the necessity of this functionality and/or how to implement it using svcs.
contextvar would solve locality issues, but if it is the default approach it might also result in non-obvious behaviour, if a user does some instantiating in a separate Task and expects the result to be carried over to other tasks. I currently do not have a specific use case requiring concurrent contexts, so my approach would be to start with a naive module-global and solve problems as they arise
yes that would be exactly it. I would add it to a next version of gimme-that :) So for now I think basing gimme-that on svcs is the way forward for me :). *mumbles something about version pinning |
Beta Was this translation helpful? Give feedback.
-
You'd think so! But I've found that in my existing projects it sometimes makes sense to just have a singular object available. You don't always want a live pool for example (for instance HTTP clients that run ~1x per day). |
Beta Was this translation helpful? Give feedback.
-
@elfjes jftr svcs 23.7 is on PyPI now that allows passing a container. I've just needed it myself in a Pyramid app! :) |
Beta Was this translation helpful? Give feedback.
-
@elfjes I'm at a point where I think svcs is ready to be pronounced stable. Do you have anything that would speak against it? There's been a lot of changes since we last spoke. 😅 |
Beta Was this translation helpful? Give feedback.
-
I don't think I'm exaggering when I say that I've spent more time on the docs than on code at this point. 😅
Well, svcs doesn't care at all – but people prefer clear instructions? I guess I can rephrase it more like a suggestion.
Hm I feel like you're off here and I wonder what to do to the docs to prevent this. Both long- and short-lived services have a perfect symmetry, because in both cases you can use
I mean you're free to keep a container longer around – they are all entirely self-sufficient as long as the registry is alive. Is anything stopping you? Containers are even context managers themselves now.
Yeah, but there's no way around that with recursive services. We could try to keep some kind of DAG to flush only dependencies of what you're flushing, but that seems a) overkill for testing and b) much harder to reason about.
The Flask integration offers only sync helpers in general. I mean technically Flask has async support too, but I don't think we need that for going stable.
I hope this works: 2597265
Thanks again for your throughtful feedback! I'm glad to see it's mostly about improving docs at this point. |
Beta Was this translation helpful? Give feedback.
-
That's good :)
Right. That still feels a bit asymmetric to me. It links the factory for a Connection, to a cleanup of an Engine. It would feel more "correct" to me to be able to write something like this: def engine_factory():
engine = create_engine("postgresql://localhost")
yield engine
engine.dispose()
def connection_factory(svcs_container):
return svcs_container.get(Engine).connect() # hurray for automatic exiting of contextmanagers
svcs.flask.register_factory(app, Engine, engine_factory)
svsc.flask.register_factory(app, Connection, connection_factory) which doesn't currently work properly because the engine would be destroyed for every request (assuming a short lived container). Then, to me, the next best thing would be: engine = create_engine("postgresql://localhost")
svcs.flask.register_value(app, Engine, engine, on_registry_close=engine.dispose) but that feels clunky. So right now, the simplest would be the pattern you suggest here and in the docs: create a higher scoped class Service:
...
@attrs.define
class MyEngine:
dep: Service
...
def create_engine_very_complex(base: Service):
engine = MyEngine(base)
# some more logic
...
return engine
def app():
"""not necessarily a flask app"""
registry = svcs.Registry()
base = Service()
engine = create_engine_very_complex(base)
registry.register_value(Service, base, on_registry_close=base.close)
registry.register_value(MyEngine, engine, on_registry_close=engine.close)
... Now I've statically tied my class ServiceA(Service):
...
class ServiceB(Service):
...
def create_service():
return ServiceA() if os.environ['SERVICE_KIND'] == 'a' else ServiceB() One of the beauties of a framework like service = create_service()
engine = create_engine_very_complex(service)
... which I was trying to avoid by using
Yes I can create multiple containers, but I would not be able to request a recursive dependency from another container. The containers are completely independent from each other.
Yeah that works against accidental production usage of overwrite :) |
Beta Was this translation helpful? Give feedback.
-
FWIW that’s what I’m doing naturally which led to the APIs.
OK, so would it be enough for you if
I feel/hope like adding containers that depend on other containers should be possible to add later? |
Beta Was this translation helpful? Give feedback.
-
Hm no this can't work with async. It might also be a bit confusing. Maybe I'll add something like But that doesn't seem like something that has to ship in the first stable version, no? |
Beta Was this translation helpful? Give feedback.
-
Yeah to be clear: I've spent so much time on the project in the past months, neglecting all my other projects, and I'm kinda running out of steam. If the current state doesn't paint me into a corner and pronouncing the current public APIs stable doesn't make our singleton dreams impossible, I think I'm gonna punt on it for now. |
Beta Was this translation helpful? Give feedback.
-
While it's true the user could do that themselves, I think it'd be a nice feature to have built in. Though I think a nicer API than the |
Beta Was this translation helpful? Give feedback.
-
Hey @hynek,
I'm the guy that did a similar thing to
svcs
(https://github.com/elfjes/gimme-that). At EuroPython you asked for some feedback, so here we go :)First of all, I like the simplicity of the registry/container design. Registries are for factories, and containers contain instances. Easy.
However, the
register_value
method seems to break this pattern. Now an instance is present at the registry-level instead of the container level. What if there'd be aContainter.add
method that adds an instance (and aContainer.remove
to remove/forget an instance) (perhaps implement__getitem__
,__setitem__
and__delitem__
) Seems to me that'd make it more clean. During testing you can then also just create a new container (fixture) for every test and don't need to mess with the registry itself.I like the ability to close/cleanup instances and the fact that it can do async cleanup. In general, having async support is nice!
Factories currently do not accept parameters. However, they can ask for other services through the 'Quality of Life' module. Would it make sense to you to allow a factory to take an (optional)
container
argument that can be used to get other services to reduce thedependency on a global object. This would also be beneficial for dealing with some locality issues, such as when using threadlocal data or
ContextVar
. You could make it a truly optional argument by inspecting the factory function to check if it requires an argument and only then supply it.The health check support seems nice, although it seems to me it pollutes both the registry and the container with something that is really specific. However, I currently do not have a better idea on how to implement it. There also seems to be some overlap with the
on_registry_close
functionality in that it allows for associating a factory with some callback. Maybe there is something there to make it cleaner?Speaking of health checks. Would it make sense to allow
ServicePing.ping
to return a value? It would allow for more detailed health checks.Finally, if you'd be open to it, we could combine our efforts and merge functionality of our two packages into one. I'm currently thinking of the following
gimme-that
. We can keep that if you wantinject
-factory function (name pending) that could be used like the following:the
inject
function would return a callable that looks at the signature/type hints and request those services from the container (requires a suppliedContainer
as an argument)Beta Was this translation helpful? Give feedback.
All reactions