Skip to content

Conversation

@guewen
Copy link
Member

@guewen guewen commented Jan 29, 2022

Migration of #154, details on the link.

You can test enqueuing a graph a jobs with test jobs by going on the runboat instance and doing a request on /queue_job/create_test_job?size=30 that will generate a random graph of 30 jobs. Jobs are not executed, but you can look at the dependency graphs ;)

This was referenced Jan 29, 2022
@guewen guewen changed the title 14.0 dependencies [14.0] Add graph of dependencies Jan 29, 2022
@guewen guewen force-pushed the 14.0-dependencies branch from d46b548 to 3ca0f6f Compare January 29, 2022 21:44
@guewen guewen force-pushed the 14.0-dependencies branch from f0b56b9 to 56be8a8 Compare March 15, 2022 14:41
@guewen guewen force-pushed the 14.0-dependencies branch from 56be8a8 to 619348e Compare May 3, 2022 15:28
Guewen Baconnier and others added 24 commits May 31, 2022 17:10
test_queue_job will create channels root.sub and root.sub.sub, so the
tests doing the same thing will fail with a unique constraint error.
When displayed in a tab, the widget show the nodes offset, the d3
network is probably confused by the size or visibility of its canvas.
Install a listener on tabs to trigger a fit() on the network.
A graph of jobs always share the same graph_uuid, which can be
used to group jobs, but is also a faster way to find all the jobs
of the graph. Then we can use the dependencies field to build the
whole graph from the pre-selection of jobs.
Jobs waiting that their dependencies are executed cannot be requeued.
They have to keep waiting their turn.
As jobs are created in the setup, but not always delayed,
the __del__ warning that the job was not delayed is triggered.
Create only the jobs necessary for each test.
* mock_jobs context manager with assert methods to check enqueued jobs
* jobs are not stored in database, yet they can be inspected and performed
  during the test
* the context manager works with graphs enqueued by delayable and with with_delay()
* direct execution of jobs works with graphs by executing them synchronously
  following their topological sort
The previous implementation was incomplete, as it was setting
the graph_uuid when `Job.add_depends()` was called. It had the
advantage of being correct when (for e.g. a test) manually created
a Job and called `add_depends()`, but it could not work when in
some situations involving a group where one of the job has no job
depending on it.

Also, remove ``Job.add_reverse_depends()`` which is not used.
guewen added 4 commits May 31, 2022 17:13
When showing large graphs, like 300 jobs (not really a real life case maybe),
the graph takes a dozen of seconds to render. Disabling the stabilization
helps a lot.
@guewen guewen force-pushed the 14.0-dependencies branch from f1d7b4e to ff8aa55 Compare May 31, 2022 15:13
guewen added 9 commits May 31, 2022 20:08
* Renaming the TestXxx models in test_queue_job is because of pytest that
  complains, confusing them for test classes
* logger.warn is deprecated in favor of logger.warning
* Add invalidation of cache in SQL update that enqueue dependent jobs
* Flush in a test, the same is done currently in the controller
* Weird one: we can no longer compare the bound methods of the models,
  compare their __func__ which is equal.
The assert fails to show the method's details if we store the __func__ in JobCall.
Anyway, it was hackish to change the content of "method".
The fact that mocks are used is an implementation detail, and is actually
hidden to the end user of the test API.
on_done() is maybe a better choice, it is less ambiguous than then() (clearer
on the fact it happens on done()) and done() (less confusion with set_done()).
I'll do the renaming soon, unless you have other suggestions.

Ref: OCA#154 (comment)
@guewen guewen force-pushed the 14.0-dependencies branch from ff8aa55 to da9133a Compare May 31, 2022 18:08
@guewen
Copy link
Member Author

guewen commented Jun 1, 2022

Hi @simahawk @sbidoul, okay to merge this?

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Skimmed quickly through it, can't find anything bad... but I trust you did your homework 😜

COOOOL THANKS!

@sbidoul
Copy link
Member

sbidoul commented Jun 1, 2022

I totally trust the author credentials :)

@simahawk
Copy link
Contributor

simahawk commented Jun 1, 2022

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-403-by-simahawk-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5206468 into OCA:14.0 Jun 1, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b8311a4. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants