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 Request] Share Caller Context (_jac_here_) with non node abilities if called from within walker #1347

Open
ChrisIsKing opened this issue Oct 7, 2024 · 18 comments
Assignees
Labels
jaclang Issues related to jac programming language jivas Issues of high priority to the Jivas Product

Comments

@ChrisIsKing
Copy link
Collaborator

Describe the feature you'd like

This feature request aims to allow non-abilities in nodes to have access to the caller context (jac_here) when invoked from within a walker. Currently, the _jac_here_ context is unavailable in non-ability functions, causing errors when trying to reference caller variables.

In many cases, a walker may need to perform some preparatory logic before deciding whether to invoke a node’s ability. During this process, it may trigger a node function that is not defined as an ability. Ideally, when this function is invoked, it should have access to the walker's context (here) by default—just like an ability does—so that the caller context is consistently available without needing to pass it as an explicit parameter or refactor the function into an ability.

Currently, this limitation forces developers to either:

  1. Pass the context (here) explicitly as a parameter, which can complicate the code.
  2. Convert the function into an ability, which may not be ideal for certain use cases where the function is not inherently intended to be an ability.

The proposed feature would remove these workarounds and streamline the walker’s logic by ensuring that any function triggered during its execution inherits the caller’s context (here), making development more intuitive and reducing unnecessary complexity.

Examples

Using this code snippet as an example:

node test_node {
    has value: int;
    has access: bool = False;

    can set_access with entry {
        if here.value < self.value {
            self.access = True;
        }
    }

    can modify_value {
        self.value = self.value * here.value;
    }
}

walker test_walker {
    has value: int = 5;

    can visit_node with root entry {
        visit [-->];
    }

    can modify_value with test_node entry {
        if (here.access) {
            here.modify_value();
        }
    }
}

with entry {
    for i in range(10) {
        root ++> test_node(value=i);
    }
    wlk = root spawn test_walker();
    print([-->]);
}

Running the above code will result in the following error:

self.value = self.value * here.value;
                               ^^^^
NameError: name '_jac_here_' is not defined

In this case, before the walker decides to invoke the modify_value ability on test_node, it performs some checks (e.g., if (here.access)). When the non-ability modify_value function is called, it does not have access to here, resulting in the error.

With the proposed feature, here.value would be accessible in the function, allowing it to execute successfully without the need to pass here as a parameter or convert modify_value into an ability. This would streamline the process and maintain logical flow without requiring extra coding steps.

@marsninja
Copy link
Contributor

Yes, this request makes sense. @kugesan1105 @ypkang @amadolid The way I would sovle this is a bit of a rearchitecture of how jac_here works. Instead of codegen producing abilities that pass the relevant object to abilities, I would create 2 new jac_rtlib (pluggin) interface for walker_here() and node_here() and codegen would resolve teh keyword to make calls to either Jac.walker_here() and Jac.node_here() in the bodies of abilities which would take no variables. This would work as a solution for this request. HOWEVER

@ChrisIsKing This is an interesting design decision in that if a method is called outside of the context of a walk the usage of here could be bug prone. Where as abilities always has a guarantee it is called in teh context of a walk. Think about it and we can discuss more.

@marsninja marsninja removed their assignment Oct 7, 2024
@ChrisIsKing
Copy link
Collaborator Author

@marsninja The thinking is that the onus is on the developer to know if the function they are calling is data spacial or not. In the above example, a dev can still call an ability outside the context of a walk. e.g

with entry {
    for i in range(10) {
        root ++> test_node(value=i);
    }
    wlk = root spawn test_walker();
    root ++> (node_obj:=test_node(value=10));
    node_obj.set_access();
}

Similarly, this will fail with the error TypeError: test_node.set_access() missing 1 required positional argument: '_jac_here_'. So, to me, the design is no different. What I want to avoid is passing the context of the walker around. In jac v1 the context was always available via the visitor keyword. I would like the same behavior in jaclang

A quick workaround I'm willing to accept is possibly a naming convention for functions we want to have inherit the caller context, possibly something like _modify_value(). This way the code gen knows that the function is a built-in designed to be called within the walker class and not outside as such it should share _jac_here_.

@marsninja
Copy link
Contributor

Ok, the good news is its very implementaable to make JacFeature.here() work dynamically, for now you can always make the walker/node a parameter of the methods. We can discuss more in person.

@ChrisIsKing ChrisIsKing added jaclang Issues related to jac programming language jivas Issues of high priority to the Jivas Product labels Oct 8, 2024
@marsninja
Copy link
Contributor

Hey @ChrisIsKing been thinking about this, and I have a proposal of some syntax that maintains semantic separation between data spatial abilities and traditional methods. How about something like

node test_node {
    has value: int;
    has access: bool = False;

    can set_access with entry {
        if here.value < self.value {
            self.access = True;
        }
    }

    can with modify_value {
        self.value = self.value * here.value;
    }
}

walker test_walker {
    has value: int = 5;

    can visit_node with root entry {
        visit [-->];
    }

    can modify_value with test_node entry {
        if (here.access) {
            here.modify_value();
        }
    }
}

Where we can have this work however there is the sytax change on the modify_value that separates it from traditional methods. If this is workable for you, i'd like to use this approach to prevent collision between desired semantic behavior and create clear separation from expression data spatial vs traditional functions. A bigger change would be to leverage def for methods and can for abilitiy semantics.

What do you think @ChrisIsKing?

@marsninja marsninja removed their assignment Oct 8, 2024
@ChrisIsKing
Copy link
Collaborator Author

ChrisIsKing commented Oct 8, 2024

@marsninja I'm fine with this syntax change if it's a quick fix. I love the idea of using def for methods and can for abilities! I imagine this won't be a simple change though, so let's do this in the interim and I can create a separate issue for that as a future roadmap item.

Who should we assign this interim fix to?

@marsninja
Copy link
Contributor

@ChrisIsKing actually it would be a quick change, it would just require code updates in all jac programs created with prior versions. I can go either way, the best path forward would probrably to give warnings of deprecation for a while. I'm down for the def / can swap. @ypkang @eldonm What are your thoughts? on using can only for abilities and using def for tradtional methods in jac.

@marsninja marsninja removed their assignment Oct 9, 2024
@eldonm
Copy link
Collaborator

eldonm commented Oct 9, 2024

So building on the thread where I see we're in agreement of distinguishing between "data spatial" and "traditional" methods/abilities. The 'def' vs 'can' are still too similar to one another and may present a situation whereby it's less than intuitive to new devs. Data spatial abilities do not need to be callable from outside the walker like a traditional method, they simply define the spatial context within which the walker should execute a block of code. I suggest we bring back (as best as we can) the code block used in JAC 1 with some adjustments. Eg.

walker test_walker {

  on visit_node with entry {
      # will execute this code while entering the visit node and provide its walker context transparently via a 'visitor' reference in the ability that is called. It is up to the dev to robustly implement the 'visitor' reference in the ability to access the walker context.
  }
  
}

@amadolid
Copy link
Collaborator

amadolid commented Oct 9, 2024

How about just declare __jac_here__ as instance variable of walker? so that we can retain here syntax and just convert to self.__jac_here__ which is technically the current_node / walker.next[-1] in spawn_call loop.

We can also assign it for node and edge to reference current walker in the same loop.

We can even put it on anchors self.__jac__.here

@eldonm
Copy link
Collaborator

eldonm commented Oct 10, 2024

@amadolid this works and we won't need the 'visitor' proposed. I'd still like to see a dedicated construct equivalent to code blocks in JAC 1, however. Using abilities for the sole purpose of controlling walker behaviour as it walks forces us to come up with clever ability names which we'll never need to call like a regular ability. I've begun using a convention like 'on_[node name]' e.g.

walker graph_walker {

    can on_root with `root entry {
        visit [-->](`?app);
    }

    can on_app with app entry {
        visit [-->](`?agents);
    }
}

We'll never need to call these like regular abilities so it'll be cleaner to distinguish this construct from regular abilities with safeguards to ensure they're not called / used like regular abilities... something like :

walker graph_walker {

    on `root with entry {
        visit [-->](`?app);
    }

    on app with entry {
        visit [-->](`?agents);
    }
}

It reads better too..

@amadolid
Copy link
Collaborator

amadolid commented Oct 10, 2024

@eldonm that looks clean. If it's optional to use, I think that's good to have
maybe some minor adjustment

walker graph_walker {
    on `root entry {
        visit [-->](`?app);
    }

    on app entry {
        visit [-->](`?agents);
    }
}

The reason I ask if it's optional because there's a scenario you want to segregate process
ex:

walker graph_walker {
    can initialize_exit with `root exit {
        # prepare for cleanup
    }

    can actual_cleanup with `root exit {
        # do some cleanup
    }
    
    can finalize_cleanup with `root exit {
        # finalize cleanup
    }
}

This should be triggered in order but in the same exit call

@eldonm
Copy link
Collaborator

eldonm commented Oct 10, 2024

@amadolid yes, definitely optional; we'd still want to use abilities as shown, where applicable.. I'm 100% on what you've suggested, let's get it!

@ChrisIsKing
Copy link
Collaborator Author

@marsninja thoughts? Looks like we want to move ahead with:

  1. Making here available in all functions/abilities by default.
  2. New syntax for non-callable/named abilities that are responsible graph traversal.

@marsninja
Copy link
Contributor

marsninja commented Oct 11, 2024

@eldonm The names would be needed for inheritance, if you wanted a derived class type to reimplement some ability functionality it would need to be named.

walker graph_walker {

    on `root with entry {
        visit [-->](`?app);
    }

   on `root with entry {
       print("Im in graph_walker");
   }

    on app with entry {
        visit [-->](`?agents);
    }
}

walker my_graph_walker:graph_walker: {
   on `root with entry {
       print("Im in my_graph_walker");
   }
}

how would you reason about overriding these abilities say you wanted to add a separate functionality for execution on root nodes, and have it be override-able in subclasses?

@marsninja
Copy link
Contributor

@ChrisIsKing lets keep discussing for a moment. So far I'm closer to agreement on the special data spatial syntax to support abilities that are triggered only by calling from other abilities, along with the separation on can/def. The reason I prefer that separation @eldonm and folks is basically all can's in that case would have the here which introduces a modularize way of adding that kind of semantic in a superset fashion to python's semantics. Abilities semantic would be separate to how methods work in python which would perfectly match the notion of having a def that is method semantics and is better aligned with the principle of supersetting python. So the narrative would be "we introduce abilites to go with your functions/methods and abiltites are denoted with can" (also guys keep in mind that in the case of a node abilities "here" is "visitor" from jac 1, in the context of a walker abilty "here" is "here" in jac 1, this was needed for consistency given the semantics of "self" i.r.t. objects).

Also @ChrisIsKing, I believe we'd ultimately want to keep the names on all abilities to be orthogonal to inheritance semantics as we'd have to create a bunch of new language rules as to what happens whne you derive classes from base classes with, lets call it anonymous abilities. Any thoughts there?

@marsninja
Copy link
Contributor

@ChrisIsKing can you add a task of documenting how abilities work to @TharukaCkasthuri's documentation roadmap doc and plan when we can have a V1. We need to capture the semantics of abilities in to documentation?

@ChrisIsKing
Copy link
Collaborator Author

@marsninja I sent the info over to Randi. In the interim while we discuss the syntax around abilities and methods, can we support the addition of making _jac_here_ available by default in all generated methods?

@ChrisIsKing
Copy link
Collaborator Author

@marsninja Following up on this

@TharukaCkasthuri
Copy link
Collaborator

@marsninja and @ChrisIsKing I'm working on adding a complete abilities example and documentation in the jac-org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jaclang Issues related to jac programming language jivas Issues of high priority to the Jivas Product
Projects
None yet
Development

No branches or pull requests

7 participants