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

Feature/ker/basic zmq rpc #2

Merged
merged 2 commits into from
Jul 3, 2024
Merged

Conversation

keriksson-rosenqvist
Copy link
Contributor

Simple ZMQ implementation of RPC.

Copy link
Contributor

@jfriel-oqc jfriel-oqc left a comment

Choose a reason for hiding this comment

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

approving, let's get this merged and start working on it from there

raise ValueError(f"No such file: {calibration_file}")
log.info(f"Loading: {calibration_file} ")
hw = Calibratable.load_calibration_from_file(str(calibration_file))
log.debug("Loaded")
Copy link
Contributor

Choose a reason for hiding this comment

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

loaded what?

Choose a reason for hiding this comment

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

@keriksson-rosenqvist Consider a context here like we do for reporting time metrics in QAT

Copy link

github-actions bot commented Jul 3, 2024

CLA Assistant Lite bot All Contributors have signed the CLA.

@keriksson-rosenqvist
Copy link
Contributor Author

I have read the Contributor License Agreement and I hereby accept the Terms.

github-actions bot added a commit that referenced this pull request Jul 3, 2024
@keriksson-rosenqvist keriksson-rosenqvist merged commit 47ee731 into main Jul 3, 2024
1 check passed
@keriksson-rosenqvist keriksson-rosenqvist deleted the feature/ker/basic_zmq_rpc branch July 3, 2024 10:57
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Copy link

@hamidelmaazouz hamidelmaazouz left a comment

Choose a reason for hiding this comment

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

Sorry, my comments came on late, leaving them here for next iteration

raise ValueError(f"No such file: {calibration_file}")
log.info(f"Loading: {calibration_file} ")
hw = Calibratable.load_calibration_from_file(str(calibration_file))
log.debug("Loaded")

Choose a reason for hiding this comment

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

@keriksson-rosenqvist Consider a context here like we do for reporting time metrics in QAT



parser = argparse.ArgumentParser(prog="QAT submission service", description="Submit your QASM or QIR program to QAT.")
parser.add_argument("program", type=str, help="Program string or path to program file.")

Choose a reason for hiding this comment

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

I presume program doesn't need a flag here right ? sth like runner --config <config> <program> ?


@property
def address(self):
return f"{self._protocol}://{self._ip_address}:{self._port}"

Choose a reason for hiding this comment

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

minor: would've leaned on the URL/URI structures here

def address(self):
return f"{self._protocol}://{self._ip_address}:{self._port}"

def _check_recieved(self):

Choose a reason for hiding this comment

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

you probably also want a retry mechanism here similar to _send() ?

thread01 = threading.Thread(
target=execute_and_check_result,
args=(client0, program, config0, {"c": { "00": 100}}),
)

Choose a reason for hiding this comment

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

any thoughts how useful would a thread pool be ? to easily scale the number of threads and stress test the clients ?

@@ -19,6 +24,9 @@ optional = true
[tool.poetry.group.licenses.dependencies]
pip-licenses = "^3.5.3"

[tool.poetry.scripts]
qat_comexe="qat_rpc.zmq.qat_commands:qat_run"

Choose a reason for hiding this comment

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

not the most intuitive name ^^, would've called it qat_run ^^

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants