Add Aggregation Feature to Flow #336
Closed
Conversation
This file contains 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
…rge feature/enable-aggregate-status-logic
bdice
reviewed
Aug 24, 2020
| return [jobs] | ||
|
|
||
| if not callable(aggregator): | ||
| raise TypeError("Expected callable for aggregator, got {}" |
Comment on lines
+67
to
+70
| if getattr(aggregator, '_num', False): | ||
| self._is_aggregate = False if aggregator._num == 1 else True | ||
| else: | ||
| self._is_aggregate = True |
There was a problem hiding this comment.
I think this is equivalent - please check it.
Suggested change
| if getattr(aggregator, '_num', False): | |
| self._is_aggregate = False if aggregator._num == 1 else True | |
| else: | |
| self._is_aggregate = True | |
| self._is_aggregate = getattr(aggregator, '_num', 0) != 1 |
| "got {}".format(type(default))) | ||
|
|
||
| def keyfunction(job): | ||
| return [job.sp.get(key, default[i]) for i, key in enumerate(keys)] |
There was a problem hiding this comment.
I'd rewrite this to use zip instead of enumerate. This might work:
Suggested change
| return [job.sp.get(key, default[i]) for i, key in enumerate(keys)] | |
| return [job.sp.get(key, default_value) for key, default_value in zip(keys, default)] |
| r"""This class handles the creation of aggregates. | ||
|
|
||
| .. note:: | ||
| This class should not be instantiated by users directly. |
There was a problem hiding this comment.
This class should be private, according to this comment.
| if len(jobs) == 1: | ||
| return str(jobs[0]) # Return job id as it's already unique | ||
|
|
||
| blob = ''.join((job.get_id() for job in jobs)) |
There was a problem hiding this comment.
I'd prefer to separate the job ids with a comma for conceptual clarity. I know this isn't really "meaningful" since it's hashed, but I think it makes the purpose clearer.
Suggested change
| blob = ''.join((job.get_id() for job in jobs)) | |
| blob = ','.join((job.get_id() for job in jobs)) |
|
To be marked as ready for review after #335 gets merged |
kidrahahjo
force-pushed
the
feature/enable-aggregate-status-logic
branch
from
e3c94ec to
619f9bf
Compare
kidrahahjo
added a commit
that referenced
this pull request
Sep 1, 2020
* Add the private Aggregate class having fundamental concepts
* Tests for _Aggregate class added
* Logic for _condition, BaseFlowOperation, FlowGroup added
* Logic extended for exec command
* Logic extended for next command
* Logic for run command
* Logic for script command added
* Logic for submit and status enabled
* Make tests pass, replaced by-job grouping with by-op grouping
* Style Fix
* Remove print statement
* Suggested changes implemented with refactoring of Aggregate class and FlowGroup
* Documentation Edit and implement suggested changes
* Improve status fetching performance
* Use get_id() instead of the id property
* Remove the use of Aggregate class and refactored
* Minute document change
* Apply suggested changes
* Deprecate eligible and complete methods of Flow{Group|Operation}
* Move property method job to JobOperation only
* Apply suggested changes
* Apply suggested changes
* Apply suggested changes
* Change variable name
* Insert assert statement to check len(_JobOperation._jobs) == 1
* Internally change the use to lists to tuples
* Additional suggested changes from code review
* Add suggested changes
* Revert accidental changes
* Use repr(job) instead of str(job) for __repr__ method and remove instance check
* Use f-strings
* Remove checks from run, submit, print_status, _generate_id methods. To be implemented in #336
kidrahahjo
force-pushed
the
feature/enable-aggregate-status-logic
branch
from
619f9bf to
5493b4b
Compare
Base automatically changed from
feature/enable-aggregate-status-logic
to
feature/enable-aggregation
November 16, 2020 15:39
|
This PR is on hold until we make improvements to |
|
Moved to #464 |
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.
Description
Introducing the use of
Aggregateclass in flow.This class includes features like:
selectparameterMotivation and Context
The current PR for aggregation is too big. Now that PR will track my project and I'll be splitting the work into several small PRs.
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary: