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

Fix startup dependencies #538

Merged
merged 4 commits into from
Apr 17, 2023
Merged

Fix startup dependencies #538

merged 4 commits into from
Apr 17, 2023

Conversation

gabegma
Copy link
Contributor

@gabegma gabegma commented Apr 12, 2023

Description:

As I was debugging, I noticed a few problems related to dependencies.

  • First if dependencies is not None would usually not work since dependencies is often an empty list. It would secede() and rejoin() potentially for nothing.
  • The dependencies were working a bit by magic, because if I would be in debug mode and go through the startup task row per row (slowly), some modules would start without waiting for their dependencies. Somewhat it wasn't causing an issue when running at normal speed, but it's safer to define the tasks in start_up_tasks so that any dependencies are already defined in the list. I added a TODO so we can improve this in the future.
  • If a startup name was a substring of another startup name, the module would wrongly identify its dependencies. I also added a TODO.
  • I increased the occurrence at which we check that the startup is done, so we don't wait longer than necessary. This should also improve some tests speed, and it improves the startup time by 20 seconds on average.

Checklist:

You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.

  • ISSUE NUMBER. You linked the issue number (Ex: Resolve #XXX).
  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • USER CHANGES. The changes are added to CHANGELOG.md and the documentation, if they impact
    our users.
  • DEV CHANGES.
    • Update the documentation if this PR changes how to develop/launch on the app.
    • Update the README files and our wiki for any big design decisions, if relevant.
    • Add unit tests, docstrings, typing and comments for complex sections.

@gabegma gabegma self-assigned this Apr 12, 2023
@gabegma gabegma force-pushed the ggm/fix-startup-dependencies branch from d4cc151 to adc1393 Compare April 12, 2023 02:43
@gabegma gabegma marked this pull request as ready for review April 12, 2023 03:01
@gabegma gabegma force-pushed the ggm/fix-startup-dependencies branch from adc1393 to 0a8ed66 Compare April 13, 2023 20:40
@gabegma gabegma changed the base branch from main to ggm/check-if-task-is-pending April 13, 2023 20:40
azimuth/startup.py Outdated Show resolved Hide resolved
@gabegma gabegma force-pushed the ggm/check-if-task-is-pending branch from f503b4c to fb3225e Compare April 17, 2023 18:55
@gabegma gabegma force-pushed the ggm/fix-startup-dependencies branch from 0a8ed66 to a9e900f Compare April 17, 2023 19:00
@gabegma gabegma force-pushed the ggm/check-if-task-is-pending branch from fb3225e to 7ea9d5a Compare April 17, 2023 19:28
@gabegma gabegma changed the base branch from ggm/check-if-task-is-pending to main April 17, 2023 19:29
@gabegma gabegma merged commit 54f4731 into main Apr 17, 2023
@gabegma gabegma deleted the ggm/fix-startup-dependencies branch April 17, 2023 20:00
gabegma added a commit that referenced this pull request Apr 19, 2023
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.

None yet

2 participants