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

Improving shard handling and testing #3

Merged
merged 15 commits into from
Oct 11, 2023
Merged

Conversation

neal-erickson
Copy link
Contributor

Shard class being filled out, including some testing for basic use cases.

ruff.toml Outdated Show resolved Hide resolved
tests/test_shard.py Outdated Show resolved Hide resolved
tests/test_sharder.py Outdated Show resolved Hide resolved
Copy link

@Jonhas-qtm Jonhas-qtm left a comment

Choose a reason for hiding this comment

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

Besides removing the lingering print statement, looks good!

self._pending_commands: dict[UnitID, list[Command]] = {}
self._shards: list[Shard] = []
print(f"Sharder created for circuit {self._circuit}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want prints vs. some other form of logging? I'm ok with it for first cut merging PR in, but I imagine we won't want to keep these prints as is.


# Handle dependency calculations
depends_upon: set[int] = set()
for shard in self._shards:
Copy link
Collaborator

Choose a reason for hiding this comment

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

wondering if dependency creation should be a self contained method. The overall _build_shard method isn't too long but this could be a natural part to break down.

Copy link
Collaborator

@peter-campora peter-campora left a comment

Choose a reason for hiding this comment

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

A couple of comments on shard.py but overall I'm in favor of merging for now to facilitate end to end progress and we can clean things up later.

@neal-erickson neal-erickson merged commit 90565f1 into main Oct 11, 2023
1 check passed
@neal-erickson neal-erickson deleted the sharding-handling branch October 11, 2023 22:38
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.

5 participants