You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2022/07/26 14:47:56 UTC
[GitHub] [buildstream] abderrahim opened a new pull request, #1706: simplify jobs
abderrahim opened a new pull request, #1706:
URL: https://github.com/apache/buildstream/pull/1706
* merge ChildJob into Job
* merge ElementJob into Job
* replace the jobs module with the jobs.py package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan merged pull request #1706: merge ChildJob into Job
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan merged PR #1706:
URL: https://github.com/apache/buildstream/pull/1706
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a diff in pull request #1706: merge ChildJob into Job
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r1306998548
##########
src/buildstream/_scheduler/job.py:
##########
@@ -322,111 +271,23 @@ async def _parent_child_completed(self, returncode):
else:
status = JobStatus.FAIL
- self.parent_complete(status, self._result)
+ self._complete_cb(self, self._element, status, self._result)
Review Comment:
I skipped this as after removing the element job, we're still using subclasses and overrides. Let's do the rename later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a diff in pull request #1706: merge ChildJob into Job
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r1306997704
##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
# process. It has some methods that are not implemented - they are meant for
# you to implement in a subclass.
#
-# It has a close relationship with the ChildJob class, and it can be considered
-# a two part solution:
-#
-# 1. A Job instance, which will create a ChildJob instance and arrange for
-# childjob.child_process() to be executed in another process.
-# 2. The created ChildJob instance, which does the actual work.
-#
-# This split makes it clear what data is passed to the other process and what
-# is executed in which process.
-#
-# To set up a minimal new kind of Job, e.g. YourJob:
-#
-# 1. Create a YourJob class, inheriting from Job.
-# 2. Create a YourChildJob class, inheriting from ChildJob.
-# 3. Implement YourJob.create_child_job() and YourJob.parent_complete().
-# 4. Implement YourChildJob.child_process().
-#
# Args:
# scheduler (Scheduler): The scheduler
# action_name (str): The queue action name
# logfile (str): A template string that points to the logfile
# that should be used - should contain {pid}.
+# element (Element): The element to work on
+# action_cb (callable): The function to execute on the child
+# complete_cb (callable): The function to execute when the job completes
# max_retries (int): The maximum number of retries
#
+# Here is the calling signature of the action_cb:
+#
+# action_cb():
+#
+# This function will be called in the child task
+#
+# Args:
+# element (Element): The element passed to the Job() constructor
+#
+# Returns:
+# (object): Any abstract simple python object, including a string, int,
+# bool, list or dict, this must be a simple serializable object.
+#
+# Here is the calling signature of the complete_cb:
+#
+# complete_cb():
+#
+# This function will be called when the child task completes
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a diff in pull request #1706: merge ChildJob into Job
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r1306998749
##########
src/buildstream/_scheduler/job.py:
##########
@@ -27,12 +27,12 @@
import traceback
# BuildStream toplevel imports
-from ... import utils
-from ..._utils import terminate_thread
-from ..._exceptions import ImplError, BstError, set_last_task_error, SkipJob
-from ..._message import Message, MessageType
-from ...types import FastEnum
-from ..._signals import TerminateException
+from .. import utils
+from .._utils import terminate_thread
+from .._exceptions import BstError, set_last_task_error, SkipJob
+from .._message import Message, MessageType
+from ..types import FastEnum
+from .._signals import TerminateException
# Return code values shutdown of job handling child processes
Review Comment:
I fixed those that I found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1706: simplify jobs
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#issuecomment-1196287855
In general I'm not fond of this refactor.
The reason why `ElementJob` is separate from the abstract `Job` class is because not all jobs are necessarily related to processing an element in the pipeline. Originally we had the *cache cleanup* job which was an example of this but has since been removed.
The way the scheduler manages *resources* (as in "processing", "downloading", "uploading") for which the user configuration can specify how much of these are allowed to occur in parallel is an API promise, I would say that any activity which buildstream is potentially processing and parallelizing should be handled by the scheduler and should be adhering to these resource restrictions.
While it is true that we do not *currently* have a non element processing job type, I'm not convinced that we will never have one, and it worries me to not have this design already sorted out and in place in case that other activities present themselves in the future (the danger being that implementors just bypass the scheduler and implement their own threads without conforming to the job pattern).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on pull request #1706: simplify jobs
Posted by GitBox <gi...@apache.org>.
abderrahim commented on PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#issuecomment-1203597928
> * I'd like to keep the separation of `Job` and `ElementJob` for reasons stated above
The main reason I decided to merge `ElementJob` into job is the fact that it didn't do much: it just replaced the abstract method to override with a callback function, and everything else was using the callbacks instead of subclassing.
It is true that there is an `Element` specific part in it, namely the fact that an `Element` is passed to the constructor (and subsequently to the callback functions). But the `Element`-specific logging stuff is already in the parent `Job` class (exposed to subclasses via protected methods).
Would you be open to reworking this patch such that the `element` passed to the job can be `None` (and even have `None` default value), and document how to make a `Job` without an `Element`?
> * I agree that the simplification of `job.py` and elimination of the separate `ChildJob` object is desirable, as I believe the first patch in this branch achieves
Ack.
> * I'm a little concerned about this patch, just because it appears to touch on task termination logic. BuildStream suspension and termination, especially in interactive scenarios, is really not very well covered in the test suite, so I would just appreciate that some creative interactive testing be done to avoid regressions on this front.
It doesn't. The only change it does is a cut-and-paste of `ChildJob.terminate()` into where it was called in `Job.terminate()`, and this is not a big function or anything.
And to be honest, this was my main motivation for touching this part of the code. I'd like to try to fix #1613, and having this back and forth between two classes isn't helping me understand.
I appreciate that this is a delicate problem (as we already have #1613 and another instance where we get a BUG message when terminating), but I think this PR isn't touching that and shouldn't be blocked by testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a diff in pull request #1706: merge ChildJob into Job
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r1306999000
##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
# process. It has some methods that are not implemented - they are meant for
# you to implement in a subclass.
Review Comment:
There are now that I reverted the second part.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] juergbi commented on a diff in pull request #1706: simplify jobs
Posted by GitBox <gi...@apache.org>.
juergbi commented on code in PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#discussion_r930627135
##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
# process. It has some methods that are not implemented - they are meant for
# you to implement in a subclass.
#
-# It has a close relationship with the ChildJob class, and it can be considered
-# a two part solution:
-#
-# 1. A Job instance, which will create a ChildJob instance and arrange for
-# childjob.child_process() to be executed in another process.
-# 2. The created ChildJob instance, which does the actual work.
-#
-# This split makes it clear what data is passed to the other process and what
-# is executed in which process.
-#
-# To set up a minimal new kind of Job, e.g. YourJob:
-#
-# 1. Create a YourJob class, inheriting from Job.
-# 2. Create a YourChildJob class, inheriting from ChildJob.
-# 3. Implement YourJob.create_child_job() and YourJob.parent_complete().
-# 4. Implement YourChildJob.child_process().
-#
# Args:
# scheduler (Scheduler): The scheduler
# action_name (str): The queue action name
# logfile (str): A template string that points to the logfile
# that should be used - should contain {pid}.
+# element (Element): The element to work on
+# action_cb (callable): The function to execute on the child
+# complete_cb (callable): The function to execute when the job completes
# max_retries (int): The maximum number of retries
#
+# Here is the calling signature of the action_cb:
+#
+# action_cb():
+#
+# This function will be called in the child task
+#
+# Args:
+# element (Element): The element passed to the Job() constructor
+#
+# Returns:
+# (object): Any abstract simple python object, including a string, int,
+# bool, list or dict, this must be a simple serializable object.
+#
+# Here is the calling signature of the complete_cb:
+#
+# complete_cb():
+#
+# This function will be called when the child task completes
Review Comment:
```suggestion
# This function will be called in the main thread when the child task completes
```
##########
src/buildstream/_scheduler/job.py:
##########
@@ -67,43 +67,57 @@ class JobStatus(FastEnum):
# process. It has some methods that are not implemented - they are meant for
# you to implement in a subclass.
Review Comment:
There are no subclasses anymore.
##########
src/buildstream/_scheduler/job.py:
##########
@@ -27,12 +27,12 @@
import traceback
# BuildStream toplevel imports
-from ... import utils
-from ..._utils import terminate_thread
-from ..._exceptions import ImplError, BstError, set_last_task_error, SkipJob
-from ..._message import Message, MessageType
-from ...types import FastEnum
-from ..._signals import TerminateException
+from .. import utils
+from .._utils import terminate_thread
+from .._exceptions import BstError, set_last_task_error, SkipJob
+from .._message import Message, MessageType
+from ..types import FastEnum
+from .._signals import TerminateException
# Return code values shutdown of job handling child processes
Review Comment:
There are still a few lines mentioning processes instead of threads. Let's fix those while we're at it.
##########
src/buildstream/_scheduler/job.py:
##########
@@ -322,111 +271,23 @@ async def _parent_child_completed(self, returncode):
else:
status = JobStatus.FAIL
- self.parent_complete(status, self._result)
+ self._complete_cb(self, self._element, status, self._result)
Review Comment:
I'd suggest renaming the `_parent_child_completed()` method as well. Especially the 'parent' term no longer makes sense. Also the doc comments should be updated to refer to threads, not processes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1706: simplify jobs
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#issuecomment-1197743747
> In general I'm not fond of this refactor.
I had further conversation with @juergbi on this and it's better to update the issue.
Main points are:
* I'd like to keep the separation of `Job` and `ElementJob` for reasons stated above
* I agree that the simplification of `job.py` and elimination of the separate `ChildJob` object is desirable, as I believe the first patch in this branch achieves
* I'm a little concerned about this patch, just because it appears to touch on task termination logic. BuildStream suspension and termination, especially in interactive scenarios, is really not very well covered in the test suite, so I would just appreciate that some creative interactive testing be done to avoid regressions on this front.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on pull request #1706: simplify jobs
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #1706:
URL: https://github.com/apache/buildstream/pull/1706#issuecomment-1223629813
Long time no reply here sorry.
> Would you be open to reworking this patch such that the element passed to the job can be None (and even have None default value), and document how to make a Job without an Element?
I'd really rather keep all the boiler plate in place which clearly separates the `ElementJob` as a specialized concrete class of the abstract `Job` class.
I know that the `Job` class has some unfortunate knowledge about elements, which is mostly related to logging context, but still it would be great to keep these separate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org