Implement passing kwarg to RPC #533 #579

Open
ramtej wants to merge 5 commits from ramtej/nac3:feature/rpc-keywords into master
Collaborator
  • Updated parameter-binding logic
  • Added demo test
- Updated parameter-binding logic - Added demo test
ramtej added 3 commits 2025-02-05 15:27:27 +08:00
mwojcik reviewed 2025-02-05 15:28:51 +08:00
@ -0,0 +19,4 @@
def run(self):
#1) All positional => a=1, b=2, c=3 -> total=6
s1 = sum_3(1, 2, 3)
if s1 != 6:
Member

use assert instead?

use ``assert`` instead?
Owner

We don't support exceptions in runkernel (yet?). One advantage of nac3artiq/demo is it can run on x86 without hardware.

We don't support exceptions in ``runkernel`` (yet?). One advantage of ``nac3artiq/demo`` is it can run on x86 without hardware.
Owner

Though of course it's also not clear how to support RPC in runkernel.

Though of course it's also not clear how to support RPC in runkernel.
sb10q requested review from derppening 2025-02-05 15:35:21 +08:00
derppening requested changes 2025-02-06 11:17:16 +08:00
derppening left a comment
Collaborator

Please address the comments, as well as run cargo fmt --all and cargo clippy --tests and fix these warnings as well.

Please address the comments, as well as run `cargo fmt --all` and `cargo clippy --tests` and fix these warnings as well.
@ -81,3 +81,2 @@
///
/// The current parallel context refers to the nearest `with parallel` or `with legacy_parallel`
/// statement, which is used to determine when and how the timeline should be updated.
/// The current parallel context refers to the nearest `with` statement, which is used to determine when and how the timeline should be updated.
Collaborator

Please reflow this line to keep within 100 characters perline.

Please reflow this line to keep within 100 characters perline.
ramtej marked this conversation as resolved
@ -373,8 +372,14 @@ impl<'b> CodeGenerator for ArtiqCodeGenerator<'b> {
fn gen_rpc_tag(
ctx: &mut CodeGenContext<'_, '_>,
ty: Type,
is_kwarg: bool, // Add this parameter
Collaborator

Is the comment necessary on the parameter?

Is the comment necessary on the parameter?
ramtej marked this conversation as resolved
@ -404,3 +409,3 @@
buffer.push(ty.len() as u8);
for ty in ty {
gen_rpc_tag(ctx, *ty, buffer)?;
gen_rpc_tag(ctx, *ty, false, buffer)?; // Pass false for is_kwarg
Collaborator

I think these comments are unnecessary, same for the changes below.

I think these comments are unnecessary, same for the changes below.
ramtej marked this conversation as resolved
@ -815,1 +821,4 @@
gen_rpc_tag(ctx, fun.0.ret, false, &mut tag)?;
use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};
Collaborator

Please move these use statements to the top of the file.

Please move these `use` statements to the top of the file.
ramtej marked this conversation as resolved
@ -848,0 +864,4 @@
param_map[0] = Some(obj_val);
pos_index = 1;
}
for (maybe_key, val_enum) in args.drain(..) {
Collaborator

I don't think drain is necessary here. You can just iterate on args and it would consume the entire Vec - Plus the mut change to the parameter is also made unnecessary.

I don't think `drain` is necessary here. You can just iterate on `args` and it would consume the entire `Vec` - Plus the `mut` change to the parameter is also made unnecessary.
ramtej marked this conversation as resolved
@ -1234,6 +1234,45 @@ impl<'a> Inferencer<'a> {
}));
}
if ["np_shape".into(), "np_strides".into()].contains(id) && args.len() == 1 {
Collaborator

Please move these changes into a separate commit, they don't belong in the same commit as the RPC changes.

Please move these changes into a separate commit, they don't belong in the same commit as the RPC changes.
ramtej marked this conversation as resolved
ramtej added 1 commit 2025-02-10 00:43:56 +08:00
ramtej added 1 commit 2025-02-10 00:50:01 +08:00
This pull request can be merged automatically.
You are not authorized to merge this pull request.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feature/rpc-keywords:ramtej-feature/rpc-keywords
git checkout ramtej-feature/rpc-keywords
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3#579
No description provided.