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 io and manager #7

Merged
merged 13 commits into from
Apr 12, 2023
Merged

split io and manager #7

merged 13 commits into from
Apr 12, 2023

Conversation

gpetretto
Copy link
Contributor

No description provided.

Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

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

Very good. There are some questions I am raising in the comments (some of them are related to my initial architectural/data structure design).
One of them is still about QResources, whether it should contain everything (and then probably called something else, but that's just a detail), or whether it should "only" contain stuff related to what the user wants as a hardware to run on, while the rest is passed down through something else (another object, or directly in arguments). Also, somehow related, it currently is a dataclass but maybe it is too constraining if we want it to be more extendable in the future (and what we actually get from that in this case is probably not worth it ?). What do you think ?
Other than that, more like cosmetic questions and one or two fixes. Not sure about the ProcessPlacement in the end, which seems more like a constraint rather than a helper (unless we allow to not set it, e.g. set it to None, maybe by default).

src/qtoolkit/core/data_objects.py Show resolved Hide resolved
src/qtoolkit/core/data_objects.py Outdated Show resolved Hide resolved
src/qtoolkit/core/exceptions.py Outdated Show resolved Hide resolved
src/qtoolkit/core/exceptions.py Outdated Show resolved Hide resolved
src/qtoolkit/host/base.py Show resolved Hide resolved
src/qtoolkit/io/pbs.py Show resolved Hide resolved
src/qtoolkit/io/pbs.py Show resolved Hide resolved
src/qtoolkit/io/base.py Show resolved Hide resolved
src/qtoolkit/io/base.py Show resolved Hide resolved
src/qtoolkit/io/base.py Show resolved Hide resolved
Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

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

I think it is ok, we can merge.

src/qtoolkit/core/data_objects.py Show resolved Hide resolved
src/qtoolkit/core/data_objects.py Show resolved Hide resolved
src/qtoolkit/core/data_objects.py Show resolved Hide resolved
@davidwaroquiers davidwaroquiers merged commit 6d054d8 into Matgenix:main Apr 12, 2023
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.

2 participants