Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎨 Raise exception instead of hiding it in finally #845

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gortibaldik
Copy link

We use the dependency-injector a lot in our private project. Currently, when the configuration of the ThreadLocalSingleton is invalid (i.e. a constructor argument is forgotten), the library prints only this exception:

UnboundLocalError: local variable 'instance' referenced before assignment

This is really frustrating, because it provides no leads to the actual error. At the company, for last 7 months, we use a fork of this library, https://github.com/gortibaldik/dependency-injector-fg with a separate PyPI repo. However as we see that this project is again maintained, we would like to merge this small PR, which solves approx 50% of all of our problems with the library (the other 50% are issues with wiring which stem from our misunderstandings... 😀 ).

@gortibaldik
Copy link
Author

@rmk135, do you have time to look at this ?

On the other hand, we have another small issue. In your pipelines you do not upload wheels to the PyPI, which makes all the builds of our private library too long. In the repo discussed above (https://github.com/gortibaldik/dependency-injector-fg) we push wheels to the PyPI. What do you think about that ?

Copy link
Contributor

@ZipFile ZipFile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Looks interesting, I'll check this more in depth later this week.

In the mean time, could you provide a real-world (or something that looks close to it) example of those 50% cases you have issues with? For documentation purposes. Since introduction of contextvars, thread-local data is not something you use often nowadays, so I would like to know your particular use case.

@gortibaldik
Copy link
Author

Are you referring to the 50% use cases where we use ThreadLocalSingleton ? We used the ThreadLocalSingleton for the database (SQLAlchemy), before we switched to the Singleton. Also for some per-thread variables in our FastAPI app.

Due to the current state of ThreadLocalSingleton (with the finally block) whenever the configuration was wrong the "unbound instance" error was raised. Basically anytime we changed per-customer configuration files the error could have risen up.

@gortibaldik gortibaldik force-pushed the gortibaldik-improve-exceptions-thread-local-singleton branch from ed5d163 to ce3baea Compare December 18, 2024 12:13
@ZipFile
Copy link
Contributor

ZipFile commented Dec 18, 2024

I see. Try experimenting with ContextLocalSingleton, this should work better in combination with FastAPI.

Regarding long build times, I do not think there will be any changes soon. It's quite delicate topic (see #487), so we should be careful here.

I'm personally waiting for 2 things:

  • ARM GHA runenrs to be available for opensource repos. ARM builds take forever to build right now due to emulation.
  • Cython 3.1. They promised better Limited API support, which may significantly reduce amount of packages we build (hence reduce build time).

@gortibaldik
Copy link
Author

Unfortunately, the project is already in production, and the experimentation is very limited. We likely won't replace ThreadLocalSingleton with ContextLocalSingleton in the forseeable future.

@alexted
Copy link

alexted commented Dec 27, 2024

Oh yeah! Understanding what's going on when something goes wrong is often the real problem with dependency-injector. Debugger is impossible to use, and the error message and the accompanying stack trace tell us nothing, don't clarify the situation, and often even cause more confusion. You have to guess, make assumptions, and try different options until this black box randomly works!

Can anything be done about it?

@gortibaldik
Copy link
Author

Hi, what is the state of this MR ? In my opinion it is really small and improves the functionality of the lib, I don't see any reasons to reject or stall this MR. Can you elaborate on this, please ?

@alexted
Copy link

alexted commented Jan 2, 2025

Dude, there are one and a half cripples working on this library. What do you think the development speed of a project like this should be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants