Implementing qkd by SoroushHoseini · Pull Request #96 · PublicQuantumNetwork/pqn-node

SoroushHoseini · 2025-07-22T19:11:39Z

Start merging the qkd logic to the master branch so we can work more on improving the code base later

Benjamin-Nussbaum

Do we need to instantiate a new Client every time an endpoint is called? Passing Clients and DeviceDrivers as arguments will increase maintainability.

Benjamin-Nussbaum · 2025-07-23T22:24:21Z

Imports should be one per line (run ruff/mypy separately on this/other scripts for now)

What do you mean? They are, ruff checked that didn't it? What file?

It doesn't check the scripts automatically at the moment because it only looks for .py files.

What is the script? What is the file? I'm pretty sure I checked everything

the script is http_server --- you can check it manually with uv run ruff check scripts/http_server. I realize this is a problem, so I will look into automating it when I'm back in town.

Benjamin-Nussbaum · 2025-07-23T23:19:27Z

+def _get_timetagger(client: Client) -> DeviceDriver:
+    if settings.timetagger is None:
+        logger.error("No timetagger configured")
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="No timetagger configured",
+        )
+
+    tagger = client.get_device(settings.timetagger[0], settings.timetagger[1])
+    if tagger is None:
+        raise HTTPException(
+            status_code=status.HTTP_404_NOT_FOUND,
+            detail="Could not find time tagger device",
+        )
+
+    logger.debug("Time tagger device found: %s", tagger)
+    return tagger


Good idea to separate out this functionality.

Return type should be TimeTaggerDevice

Add arguments for the settings values (eliminates None check and decouples from a global settings object.

Double check elsewhere in the file that if there is a tagger, it was retrieved via this function (single source of truth)

Consider similar functions for other Devices

Not sure how to do this except for changing the return type, how would you recommend doing that?

def _get_timetagger(client: Client, arg_describing_settings.timetagger[0], same_for_1) -> TimeTaggerDevice: tagger = client.get_device(arg_desc..., same_for...) if tagger is None: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail="Could not find time tagger device", ) logger.debug("Time tagger device found: %s", tagger) return tagger

something like that, with descriptive argument names and type hints. Users unpack settings.timetagger on the calling side.

Benjamin-Nussbaum · 2025-07-23T23:33:03Z

+async def _count_coincidences(
+    measurement_config: MeasurementConfig,
+    tagger: DeviceDriver | None = None,
+    tagger_address: str | None = None,
+    http_client: httpx.AsyncClient | None = None,
+) -> int:
+    if tagger is None and tagger_address is None:
+        msg = "Either tagger or tagger_address must be provided"
+        raise ValueError(msg)
+
+    if tagger_address is not None and http_client is None:
+        msg = "http_client must be provided if tagger_address is provided"
+        raise ValueError(msg)
+
+    if tagger_address is None:
+        assert tagger is not None
+        assert hasattr(tagger, "measure_coincidence")
+        count = tagger.measure_coincidence(
+            measurement_config.channel1,
+            measurement_config.channel2,
+            measurement_config.binwidth,  # might have to cast to int
+            int(measurement_config.duration * 1e12),
+        )
+    else:
+        assert http_client is not None
+        r = await http_client.get(
+            f"http://{tagger_address}/timetagger/measure?duration={measurement_config.duration}&binwidth={measurement_config.binwidth}&channel1={measurement_config.channel1}&channel2={measurement_config.channel2}&dark_count={measurement_config.dark_count}"
+        )
+        # TODO: Handle other status codes
+        if r.status_code != status.HTTP_200_OK:
+            raise HTTPException(
+                status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                detail="Failed to measure coincidences",
+            )
+        count = cast("int", r.json())
+        if not isinstance(count, int):
+            logger.error("Invalid response from timetagger: %s", count)
+            raise HTTPException(
+                status_code=status.HTTP_406_NOT_ACCEPTABLE,
+                detail="Invalid response from timetagger",
+            )
+        logger.debug("Measured %d coincidences", count)
+    return int(count)


This function is trying to do too many things. I suggest just passing the tagger itself, in which case the type hint should be TimeTaggerDevice. If users want to get the tagger from the address, they should use the existing function get_timetagger.

Now that I think about it, it might be better to be an endpoint, it would be good for a timetagger node to be available to return coincidence counts to anyone who wants to ask. Not sure if we already have something like that.

This was something implemented by @marcosfrenkel , so he should probably comment on why, but I think the reasoning is that you want to make sure the timetagger is connected and all good before you start a protocol, and all of these checks were repeated a lot before he made this function.

Benjamin-Nussbaum · 2025-07-23T23:50:43Z

-settings = Settings("", "", 0, CHSHSettings())
+static_typecheck_msg = "Please set the global 'settings' variable before use."
+
+
+def get_settings() -> Settings:
+    raise NotImplementedError(static_typecheck_msg)
+
+
+settings = get_settings()


get_settings() should presumably parse a config file or similar so as to actually return something useful

What is that line "settings = Settings("", "", 0, CHSHSettings())"? That does not appear in my code. Also, the function is only meant for ruff / mypy checks, we put in the settings separately. I am pretty sure we can in a future PR implement settings from config files, probably in a PR where we finally handle state better and handle devices better. Though I am not sure if mypy / ruff could parse the config file reading anyway, we might need the base code to have something like this, then whoever is installing this has to make their settings config.

That's the diff showing what was removed in this changeset.

Anything included as "special treatment" just to satisfy CI checks is treating the symptom rather than the disease. If this patch ends up being included in the final changeset anyway, just be aware that it is additional technical debt that will need to be addressed.

How do we deal with it though? I am pretty sure if we had get settings make settings from a config file, then mypy and ruff would stiff through a fit? Maybe have it check for a config file to pull the settings file from, and if not riase the not implemented error?

My initial guess as to why it's complaining if you just return Settings() is because there are required arguments. That issue can be fixed by specifying default values for all fields in Settings.

SoroushHoseini · 2025-07-24T00:02:55Z

Do we need to instantiate a new Client every time an endpoint is called? Passing Clients and DeviceDrivers as arguments will increase maintainability.

The problem is we can't pass clients or device drivers into a function that is expected to be called via html since they only take json-able objects.

Benjamin-Nussbaum · 2025-07-24T00:37:27Z

Do we need to instantiate a new Client every time an endpoint is called? Passing Clients and DeviceDrivers as arguments will increase maintainability.

The problem is we can't pass clients or device drivers into a function that is expected to be called via html since they only take json-able objects.

Then maybe just refactor to use a single function (as with get_timetagger()) for now.

Benjamin-Nussbaum · 2025-07-24T00:44:40Z

-    angle = state.chsh_basis[index] + 90 * perp
-    assert hasattr(hwp, "move_to")
+    angle = state.chsh_request_basis[index] + 90 * perp
+    hwp = cast("Any", hwp)


It seems like hwp = cast("Any", hwp) is not really saying anything. If this Device were properly parsed, the typechecker would be aware of the type and available methods.

Whoops, thought I deleted that one, my bad

SoroushHoseini · 2025-07-24T01:08:13Z

Do we need to instantiate a new Client every time an endpoint is called? Passing Clients and DeviceDrivers as arguments will increase maintainability.

The problem is we can't pass clients or device drivers into a function that is expected to be called via html since they only take json-able objects.

Then maybe just refactor to use a single function (as with get_timetagger()) for now.

Now this could be me being very wrong for future config file making, but what if we make a a client and store it in the settings state and use that every time? Then again, I could see it being a problem trying to serialize it later. Clients are designed to be fairly ephemeral objects anyway though, is there a big issue in making one for the purpose of a function then killing it? @marcosfrenkel would have to chime in but I think that is how they were designed.

Benjamin-Nussbaum · 2025-07-24T03:07:37Z

Now this could be me being very wrong for future config file making, but what if we make a a client and store it in the settings state and use that every time? Then again, I could see it being a problem trying to serialize it later. Clients are designed to be fairly ephemeral objects anyway though, is there a big issue in making one for the purpose of a function then killing it? @marcosfrenkel would have to chime in but I think that is how they were designed.

In that case, we should make it an annotated dependency argument using FastAPI Depends as we've defined for the http client. See the definition at https://github.com/PublicQuantumNetwork/pqn-stack/blob/1843b58cea32d5ef6b8cff46259ee696075c733e/src/pqnstack/app/main.py#L74 and various uses throughout the file.

marcosfrenkel and others added 7 commits July 10, 2025 15:41

First draft of qkd implementation

21b1bbc

Fixing any errors present in qkd

2423603

Simplifying bitstring creation

c797304

qkd bugfixes

af1bcfd

Remove Settings from main.py

1a5be94

Fix all Ruff problems

c921cf9

Fix mypy errors

07cf999

Benjamin-Nussbaum requested changes Jul 23, 2025

View reviewed changes

Benjamin-Nussbaum reviewed Jul 24, 2025

View reviewed changes

Fix hwp cast

1843b58

SoroushHoseini requested a review from marcosfrenkel July 24, 2025 01:08

Fix ruff checks on http_server script

df6e492

marcosfrenkel approved these changes Jul 24, 2025

View reviewed changes

marcosfrenkel merged commit 25af059 into master Jul 24, 2025
6 checks passed

marcosfrenkel deleted the implementing_qkd branch July 24, 2025 19:18

marcosfrenkel mentioned this pull request Jul 25, 2025

Decouple protocols from main.py #97

Closed

Benjamin-Nussbaum mentioned this pull request Jul 25, 2025

Modularize api #100

Merged

Conversation

SoroushHoseini commented Jul 22, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SoroushHoseini commented Jul 24, 2025

Uh oh!

Benjamin-Nussbaum commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SoroushHoseini commented Jul 24, 2025

Uh oh!

Benjamin-Nussbaum commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Benjamin-Nussbaum Jul 24, 2025 •

edited

Loading

Benjamin-Nussbaum commented Jul 24, 2025 •

edited

Loading