Unify group and aggregate storage with a bi-map data structure.

[bdice]

Description

Introduces a bidirectional map data structure that allows for a unification of group / aggregate registration and storage mechanisms in the FlowProject.

Motivation and Context

Using a bidict data structure reduces redundancy. It eliminates superfluous private methods and makes all accesses O(1) whether by group or by aggregator.

This is an intermediate step for unifying and optimizing the status fetching logic with run/submit logic.

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the API documentation as part of the package doc-strings.
• I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
• I have updated the changelog.

[atravitz]

This seems much more intuitive and efficient! I just have a few things I'd like to understand

[atravitz]

this and line 2186 are where we get the performance boost, yes?

[bdice]

That is a small speedup, yes. The bigger performance boost is from replacing code like:

for group in groups:
    for aggregate in self._get_aggregate_store(group.name).values()
        ...

with the selection logic that iterates over aggregates, then groups. This is partially implemented in this PR and more thoroughly implemented in #416.

[bdice]

There are some template tests that are failing due to (nondeterministic?) re-ordering of operations that run in parallel. From what I can tell, we don't enforce a sorting order for the output script when operations are launched in parallel. I think this choice of ordering is arbitrary (it shouldn't matter for execution) but we may want to enforce a sorting order (e.g. sorted by the _JobOperation id) to prevent template test failures.

[kidrahahjo]

@bdice this is fantastic. I loved the way it is implemented.

One question, the reason to store values like before was due to memory issues that can come into play. Now we're storing the mapping of the group to its aggregate store ({group1: default_store. group2: default_store}) as well as the mapping of an aggregate store to all the groups it's associated to ({default_store: [group1, group2]}).

Wouldn't this be a problem?

[kidrahahjo]

There are some template tests that are failing due to (nondeterministic?) re-ordering of operations that run in parallel. From what I can tell, we don't enforce a sorting order for the output script when operations are launched in parallel. I think this choice of ordering is arbitrary (it shouldn't matter for execution) but we may want to enforce a sorting order (e.g. sorted by the _JobOperation id) to prevent template test failures.

I agree with @bdice. While analysing the test failure output, I saw that when bundling, the order of operations is not the same as intended. A quick fix would be something like this:

for bundle in _make_bundles(operations, bundle_size):
  def key_func(op):
      return op._id
  bundle = sorted(bundle, key=key_func)

But I am not sure if we should go in this direction but it's a small start.

[b-butler]

I like the use of the bidirectional mapping.

[b-butler]

Do we need setdefault here? If we set self.inverse[self[key]] = [], then remove will always just raise an ValueError. self.inverse[self[key]] should exist or the class would be in an invalid state.

[bdice]

This might be true. I got this implementation from a StackOverflow post and cleaned up a few pieces of it, but I don't have any tests for the class. I think your statement seems reasonable and changing that worked in my small amount of local testing. I also looked at the StackOverflow post comments but nobody had mentioned that piece of the code. Fixed in adf5370.

[vyasr]

I agree with Brandon's analysis. setdefault is required in __setitem__ and in the constructor (to account for duplicate keys).

[vyasr]

I'm fine with the contents of this PR and with how my comment was resolved, but @atravitz, @b-butler, and @kidrahahjo have given this a more thorough review than I have so I'm going to remove myself as a reviewer and defer to their evaluations rather than approving.

[bdice]

I fixed the template tests in #416. Reviewers, please ignore the failing tests on this PR.

[kidrahahjo]

I have some minor suggestions/questions, apart from them, the PR is looking good.

[kidrahahjo]

Suggested change

	for aggregate in aggregate_store.values():
	for aggregate in aggregate_store.values():
	if aggregate not in aggregates:
	continue

This is a really small optimization which makes a difference when only a few aggregates/jobs are provided by the user.

Given this, we can now remove the exact same check in the select method above.

[bdice]

I will look at this in #416, I refactored this code and may have already made the change you're suggesting.

[bdice]

I think the new approach in #416 is similar to what you were looking for here. Please comment on that PR in the method _generate_selected_aggregate_groups if not. Here's the logic:

if selected_aggregates is not None:
    # Use selected aggregates in the aggregate store
    for aggregate in aggregate_progress_wrapper(selected_aggregates):
        aggregate_id = get_aggregate_id(aggregate)
        if aggregate_id in aggregate_store:
            for group in matching_groups:
                yield aggregate_id, aggregate, group
else:
    # Use all aggregates in the aggregate store
    for aggregate_id, aggregate in aggregate_progress_wrapper(
        aggregate_store.items()
    ):
        for group in matching_groups:
            yield aggregate_id, aggregate, group

[kidrahahjo]

This is not group-specific so we can consider checking this in the outer loop and continue if it's not the selected aggregate.

Also, I feel like the _is_selected_aggregate method is now redundant because we simply just check the availability of aggregate in aggregates.

[bdice]

This is handled in #416.

[kidrahahjo]

Do we have tests to check that the groups don't get executed unless the user specifically asked for them?

[bdice]

I'm not entirely sure but I think we have coverage of that. I'll defer this to a follow-up PR.

[bdice]

I confirmed that tests fail on #416 if this check is not performed correctly (if more groups are run/status-fetched/submitted than were requested).

[b-butler]

@bdice Also do note that update will not call __setitem__, pop will not call __delitem__, setdefault will not call __setitem__ and so on. The CPython dict class is optimized to not use these paradigms that exist in abc.collections. We should probably either disable these methods return NotImplementedError or implement all methods that involve setting or deleting. I forgot this when reviewing yesterday.

[bdice]

@b-butler That's a really helpful observation, I didn't realize that. I re-implemented the class as a MutableMapping with an internal dictionary, so it gets all of those methods "for free." Also, I added tests for all of the methods expected from a MutableMapping. Fixed in 0789f2a.

[b-butler]

I left a comment about the implementation of _bidict. Also I would like to test some of the methods mentioned in the comment in your _bidict test. Beyond that I am ready to merge.

[bdice]

I'm going to go ahead and merge this into next. I will handle the remaining suggestions/comments in #416. The two PRs are very closely related and I think #416 will fix most of the problems that we have identified in this PR. Thank you so much to all reviewers for the thoughtful input!