Skip to content

add ability to specify custom instruction generator function#183

Merged
nreinicke merged 3 commits intomainfrom
ndr/129-instruction-gen-callable
Apr 25, 2023
Merged

add ability to specify custom instruction generator function#183
nreinicke merged 3 commits intomainfrom
ndr/129-instruction-gen-callable

Conversation

@nreinicke
Copy link
Copy Markdown
Collaborator

Closes #129 by adding the ability to specify custom control logic as either a InstructionGenerator or an InstructionFunction which is any callable that has the signature Callable[[SimulationState, Environment], Tuple[Instruction, ...]]

@robfitzgerald I'm interested to hear if this satisfies what you had in mind with #129

@nreinicke nreinicke requested a review from robfitzgerald April 25, 2023 17:17
Copy link
Copy Markdown
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

thanks nick, this is the idea. i think making these APIs callables vs requiring them to derive a base class is a little more pythonic for people who want to easily add new control logic in hive.

i think there's a complication that may require a little more thought around identifying the unique AnonGenerators, along with another small request to finish the test.

rp = load_simulation(conf, custom_instruction_generators=[custom_instruction_function])

result = crank(rp, 1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this test should include an assertion, like, perhaps the custom function creates a fixed set of instructions that can be confirmed at the end

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's a good call, I updated the test to make sure the instructions are getting applied and using two different functions to make sure the name property is working

self.instruction_function = instruction_function

@property
def name(self) -> str:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think you set up instruction generators to be stored in a Map[str, InstructionGenerator] where the key comes from this name. if that's the case, this design fails if there are more than one anonymous InstructionGenerators added to the system.

two ideas:

  • while looping through instruction_generator_from_function below, enumerate the iterable and append the index number when creating the name in the init of AnonGenerator
  • make users responsible for providing a name, which is probably the right move, though it means they have to pass along a Tuple[str, Callable[etc]] instead of just the callable

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

aha yes, good catch; how about if we use the incoming function name?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that's going to be something generated by the interpreter like "lambda$a2354h8-aggahds" or something like that, right? is that helpful for the user? again, if there is a runtime error in their code, it might not be clear which of multiple IGs is the one that's responsible.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's a good start though. maybe i should back off and let that through and we worry about fixing it if people have issues later.

Copy link
Copy Markdown
Collaborator

@robfitzgerald robfitzgerald Apr 25, 2023

Choose a reason for hiding this comment

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

it's a good start though. maybe i should back off and let that through and we worry about fixing it if people have issues later. what do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the names actually get propagated as how they're written in python; for example, if I inspect the StepSimulation object in the unit test I get:

image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that looks good enough to me, as it should likely correspond to something the user can make sense of. thanks for confirming!

Copy link
Copy Markdown
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

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

🐝

@nreinicke nreinicke merged commit 3fd2a1c into main Apr 25, 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.

add an api to inject InstructionGenerators as an anonymous function

2 participants