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

TDQM hook with overrides #845

Open
zilto opened this issue Apr 25, 2024 · 5 comments
Open

TDQM hook with overrides #845

zilto opened this issue Apr 25, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest Hacktoberfest issues hacktoberfest2024 hacktoberfest 2024

Comments

@zilto
Copy link
Collaborator

zilto commented Apr 25, 2024

Current behavior

When using hamilton.plugins.h_tdqm.ProgressBar(), the progress bar uses "the number of nodes to return" as denominator and "the number of nodes executed" as numerator. However, passing values to .execute(overrides=...) counts towrds the first, but not the latter. Code runs to completion but the progress bar completes without reaching 100%

Screenshots

image

Dataflow

# joke.py

def joke_prompt(topic: str) -> str:
    return f"Knock, knock. Who's there? {topic}"

def reply(joke_prompt: str) -> str:
    _, _, right = joke_prompt.partition("? ")
    return f"{right} who?"

def punchline(reply: str) -> str:
    left, _, _ = reply.partition(" ")
    return f"No, {left} MooOOooo"

Execution

import joke
from hamilton import driver
from hamilton.plugins.h_tqdm import ProgressBar
dr = (
    driver.Builder()
    .with_adapters(ProgressBar())
    .with_modules(joke)
    .build()
)

dr.execute(["joke_prompt", "reply"], overrides=dict(punchline="..."), inputs=topic("hello")).
@zilto zilto added triage label for issues that need to be triaged. bug Something isn't working good first issue Good for newcomers and removed triage label for issues that need to be triaged. labels Apr 25, 2024
@elijahbenizzy elijahbenizzy removed the good first issue Good for newcomers label Apr 25, 2024
@elijahbenizzy
Copy link
Collaborator

Hmm this might be a bit tricky -- we'll probably want to add a get_execution_plan() for the HamiltonGraph object. Otherwise we won't have access to the number. Otherwise we'll also want a overrides in the GraphExecutionHook (

class GraphExecutionHook(BasePreGraphExecute, BasePostGraphExecute):
).

Everything will be backwards compatible, but not super simple.

@zilto
Copy link
Collaborator Author

zilto commented Apr 26, 2024

This seems low priority, but good to have it documented! Thanks for the details

@skrawcz
Copy link
Collaborator

skrawcz commented Jul 18, 2024

We could just make sure there's a post graph hook implementation that makes sure the bar hits 100% if there was no error?

@rajatkriplani
Copy link

Hi @skrawcz following up from issue #1186
and @zilto I have come up with the following:

  • adjusting pre_graph_execute by adding the overridden node to execution path to reflect progress:
nodes_to_execute = set(all_nodes) - set(user_defined_nodes) | set(overrides.keys())

OR

nodes_to_execute = set(all_nodes) 
  • in post_graph_execute force progress bar to 100% if the execution is successful:
if success and hasattr(self, 'progress_bar'):
       self.progress_bar.update_to_completion()

@skrawcz
Copy link
Collaborator

skrawcz commented Oct 18, 2024

@rajatkriplani Cool feel free to create a PR with it. If there isn't a test we'll need to add one.

@skrawcz skrawcz added hacktoberfest Hacktoberfest issues hacktoberfest2024 hacktoberfest 2024 labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest Hacktoberfest issues hacktoberfest2024 hacktoberfest 2024
Projects
None yet
Development

No branches or pull requests

4 participants