Unify and improve config option registration code #5234
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pull-request-size
bot
added
the
size/L
all the functions take "ignore_errors" argument. For consistnecy, also pass "ignore_errors=True" in places where we parse options as import time side affects (nasty, but that code has a lot of legacy and removing import time side affects would be a large undertaking). Also add not so ideal workaround for "config option already registered" we occasionaly see. Sadly the workaround is not ideal, but per comment above, real fix would require large refactoring + much bigger time investement.
Kami
force-pushed
the
unify_config_options_registration
branch
from
d0141e3 to
17f4006
Compare
Kami
force-pushed
the
unify_config_options_registration
branch
from
17f4006 to
8418223
Compare
number of children since rest of the code asserts that 2 childrens are returned. Previously version of code would already return in case there is just 1 children which means the tests would fail and be prone to timing / race conditions.
|
I'll merge this since it also contains a test fix (#5227 (comment)) and the change itself should be safe and fully backward compatible - this way we can get some "CI miles" on it as part of nightly job and see how it behaves over multiple build runs. |
Kami
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to unify and make all the config registration code more consistent - aka so it exposes / utilizes the same API.
I only started to dig into since I started a bunch of test errors like that recently - https://gist.github.com/Kami/fcfa62f1ae7f0b84cd3981ecd6f5fe37.
It looks like there is some kind of race with config option registration which likely depends on the import ordering or similar. Either this issue has been there for a long time already (not so unlikely) or some recent code changes re-arranged the imports or similar which started to trigger this issue more often (I did quickly check git history without much success., but I could have also missed something)
Sadly most of the config modules itself have import time side affects (options are registered on import) which is bad. Fixing / improving that would likely be a very deep rabbit hole since tons of existing code and tests relies on this behavior.
So with this PR I'm really just trying to unify the code and gets tests to pass consistently. Keep in mind that registering the same options multiple times should not be and it's not fatal so if we ignore such errors it's OK (well, it's not prefect, but it's compromise - an alternative is much larger code change which could likely takes many days to even weeks).