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

Split transactron core #617

Merged

Conversation

lekcyjna123
Copy link
Contributor

One more try to split transactron/core.py. I have started from scratch, because from the last one passed half a year. There was no changes in the code except imports and changing objects in types to type strings.



@dataclass(frozen=True)
class TransactionManagerKey(SimpleKey["TransactionManager"]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if string in generic argument to SimpleKey is a feature, but it works.

@tilk tilk added the refactor Doesn't change functionality, but makes stuff nicer label Mar 12, 2024
Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

That's not a bad split. I sure hope this PR foretells the end of the refactor season ;)

@tilk
Copy link
Member

tilk commented Mar 12, 2024

This is related to #446.

Copy link
Member

@piotro888 piotro888 left a comment

Choose a reason for hiding this comment

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

The split structure also makes sense for me, everything is more readable now :)

EDIT: Shouldn't TransactionManagerKey be placed in core/manager.py instead of core/transaction.py? [I'll approve after answer to that point]

For some bikeshedding:
I don't think that eager_deterministic_cc_scheduler, trivial_roundrobin_cc_scheduler and TransactionManagerKey shold be imported by default from transactron import * (if we care about default imports structure at all).


Not in the scope of this PR but before we separate transactron, I have two concerns about lib and utils:

  • _typing.py -> typing.py - Underscore is misleading that it contains internal transactron types, while it has many useful types for Amaranth or use of transactron concepts externally in designs (also # Internal Coreblocks types :) )
  • utils/transactron_helpers.py - is it supposed to contain functions used only internally in transactron? Now some of that functions look like that, but others (like functions for layouts) are used in Coreblocks and are useful for external use. I would split that to utils/_transactron_utils.py (with emphasized private underscore) and second file.

@lekcyjna123
Copy link
Contributor Author

EDIT: Shouldn't TransactionManagerKey be placed in core/manager.py instead of core/transaction.py?

That was my initial idea, but this cause cyclic dependency. TransactionManager needs Transaction and Transaction needs TransactionManagerKey.

Other possibility is to move TransactionManagerKey to separate file, but this would be overkill in my opinion.

@lekcyjna123
Copy link
Contributor Author

I don't think that eager_deterministic_cc_scheduler, trivial_roundrobin_cc_scheduler and TransactionManagerKey

Theoretically transactron was written with possibility to use user own instances of TransactionManager with chosen scheduler. That is the reason why I decided to export the symbols.

@lekcyjna123 lekcyjna123 mentioned this pull request Mar 13, 2024
@piotro888
Copy link
Member

but this cause cyclic dependency.

:(

Other possibility is to move TransactionManagerKey to separate file, but this would be overkill in my opinion.

I would be for separate file then (maybe transactron/core/keys.py ?). TransactionManagerKey is used externally from Transactron lib and its import location from transactron.core.transactions is in my opinion confusing.
Maybe some other keys would be used too in future and they usually cause cyclic dependency problems.

Theoretically transactron was written with possibility to use user own instances of TransactionManager with chosen scheduler. That is the reason why I decided to export the symbols.

My point is that is should be imported via from transactron.core.schedulers import * and not pollute the from transactron import *. If implementing this is problematic or breaks documentation generation, current solution is fine then.

@lekcyjna123
Copy link
Contributor Author

I would be for separate file then (maybe transactron/core/keys.py ?)

Moved.

My point is that is should be imported via from transactron.core.schedulers import * and not pollute the from transactron import *. If implementing this is problematic or breaks documentation generation, current solution is fine then.

Done.

@tilk tilk requested a review from piotro888 March 17, 2024 12:36
Copy link
Member

@piotro888 piotro888 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tilk tilk merged commit 044b125 into kuznia-rdzeni:master Mar 17, 2024
8 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 17, 2024
tilk pushed a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Doesn't change functionality, but makes stuff nicer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants