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

Additional updates to parallelism docs #453

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented Dec 4, 2024

More updates to make API consistent:

  1. Showing inputs for class-based action (repeat but good to hammer home)
  2. Making API consistent

Important

Updates parallelism documentation for consistent API and input handling, with minor debugging code addition.

  • Documentation Updates:
    • Emphasize input handling for class-based actions in parallelism.rst.
    • Add inputs property examples in parallelism.rst for parallel actions.
    • Minor updates in actions.rst to reinforce class-based action inputs.
  • Code Changes:
    • Add pdb.set_trace() in run_and_update() in parallelism.py for debugging.

This description was created by Ellipsis for 6abea5c. It will automatically update as commits are pushed.

@elijahbenizzy elijahbenizzy force-pushed the improve-parallelism-docs branch from 87bb48e to 95c5a8c Compare December 4, 2024 17:14
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 95c5a8c in 28 seconds

More details
  • Looked at 120 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_Hg452Bk5TmtutZBO


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

return ["prompts"] + super().inputs # make sure to include the superclass inputs

def states(self, state: State, context: ApplicationContext, inputs: Dict[str, Any]) -> Generator[Tuple[State, dict], None, None]:
for prompt in inputs["prompts"]
Copy link

Choose a reason for hiding this comment

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

Missing colon at the end of the for loop.

Suggested change
for prompt in inputs["prompts"]
for prompt in inputs["prompts"]:

Copy link

github-actions bot commented Dec 4, 2024

A preview of 6abea5c is uploaded and can be seen here:

https://burr.dagworks.io/pull/453

Changes may take a few minutes to propagate. Since this is a preview of production, content with draft: true will not be rendered. The source is here: https://github.com/DAGWorks-Inc/burr/tree/gh-pages/pull/453/

@elijahbenizzy elijahbenizzy force-pushed the improve-parallelism-docs branch from 95c5a8c to be539f1 Compare December 4, 2024 17:16
More updates to make API consistent:

1. Showing inputs for class-based action (repeat but good to hammer
   home)
2. Making API consistent
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on be539f1 in 50 seconds

More details
  • Looked at 129 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. docs/concepts/parallelism.rst:167
  • Draft comment:
    The state method should include context and inputs parameters for consistency.
            def state(self, state: State, context: ApplicationContext, inputs: Dict[str, Any]) -> State:
  • Reason this comment was not posted:
    Marked as duplicate.
2. docs/concepts/parallelism.rst:375
  • Draft comment:
    The action method should include state, context, and inputs parameters for consistency.
        def action(self, state: State, context: ApplicationContext, inputs: Dict[str, Any]) -> Action | Callable | RunnableGraph:
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_zXaGU9PCJHfDxls2


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

docs/concepts/parallelism.rst Outdated Show resolved Hide resolved
docs/concepts/parallelism.rst Show resolved Hide resolved
@elijahbenizzy elijahbenizzy force-pushed the improve-parallelism-docs branch from be539f1 to 6abea5c Compare December 4, 2024 17:18
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 6abea5c in 31 seconds

More details
  • Looked at 129 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. docs/concepts/parallelism.rst:81
  • Draft comment:
    Avoid using pdb.set_trace() in production code as it can halt execution unexpectedly. Consider using logging or other debugging techniques instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_94hIhZblzAggb8pe


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@elijahbenizzy elijahbenizzy merged commit ddd675a into main Dec 4, 2024
11 checks passed
@elijahbenizzy elijahbenizzy deleted the improve-parallelism-docs branch December 4, 2024 19:10
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.

1 participant