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

fix: Improve error message for SQLitePersister #417 #418

Merged

Conversation

arpitgupta-it
Copy link
Contributor

@arpitgupta-it arpitgupta-it commented Nov 8, 2024

[Short description explaining the high-level reason for the pull request]

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Important

Add initialization check for persisters in with_state_persister to prevent runtime errors with uninitialized persisters.

  • Behavior:
    • Add check in with_state_persister in application.py to raise RuntimeError if persister is not initialized.
  • Persistence:
    • Add _initialized attribute and is_initialized() method to SQLLitePersister in persistence.py to track initialization state.

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

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! Reviewed everything up to 5e31502 in 39 seconds

More details
  • Looked at 40 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. burr/core/application.py:2212
  • Draft comment:
    The error message could be more concise and consistent with other error messages in the codebase. Consider rephrasing it for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a check for the initialization of the persister before using it. This is a good practice to ensure that the persister is ready to be used. However, the error message could be improved for clarity and consistency.

Workflow ID: wflow_OXqUxnREvdXATx7U


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

burr/core/application.py Outdated Show resolved Hide resolved
Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

on the right track, some more comments!

burr/core/persistence.py Outdated Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
@arpitgupta-it arpitgupta-it force-pushed the SQLLitePersister-Improvement branch from 101b88b to f8e3145 Compare November 16, 2024 13:23
@arpitgupta-it
Copy link
Contributor Author

Suggested changed added!

So this makes the change more robust.
We need to special case and ensure that

1. existing behavior doesn't break (not ImplementedError is the default)
2. new behavior works - added tests.

I decided that redis and mongodb don't need this,
but if it is an issue we can always add it.
Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

I think things look good. Asking @elijahbenizzy for review.

Copy link
Contributor

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Looking good! Thank you! Left a few comments -- should be easy to fix up.

burr/core/application.py Show resolved Hide resolved
burr/core/persistence.py Show resolved Hide resolved
burr/core/application.py Show resolved Hide resolved
burr/core/persistence.py Show resolved Hide resolved
burr/core/persistence.py Outdated Show resolved Hide resolved
burr/core/persistence.py Outdated Show resolved Hide resolved
@arpitgupta-it
Copy link
Contributor Author

Hi @elijahbenizzy and @skrawcz,

I’ve reviewed the feedback and the discussions above, but I’m a bit unsure about the specific changes that still need to be made. Could you please clarify what needs to be updated or adjusted in the PR based on the comments so far?

I want to make sure everything is aligned before moving forward with the changes.

Thanks!

After checking if the table exists we can probably safely
assume things have been initialized properly, and we
can then set the initialized flag appropriately.
@skrawcz
Copy link
Contributor

skrawcz commented Nov 19, 2024

Hi @elijahbenizzy and @skrawcz,

I’ve reviewed the feedback and the discussions above, but I’m a bit unsure about the specific changes that still need to be made. Could you please clarify what needs to be updated or adjusted in the PR based on the comments so far?

I want to make sure everything is aligned before moving forward with the changes.

Thanks!

They were very minor things -- you can see my commit for them; thought that would be easier than explaining. I think we should be good to go after these changes.

@arpitgupta-it
Copy link
Contributor Author

Thanks for handling the updates @skrawcz ! Looks like we’re all set... 🤞

Copy link
Contributor

@elijahbenizzy elijahbenizzy 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!

@elijahbenizzy elijahbenizzy merged commit 988b9fb into DAGWorks-Inc:main Nov 21, 2024
10 of 11 checks passed
@arpitgupta-it arpitgupta-it deleted the SQLLitePersister-Improvement branch November 22, 2024 03:47
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.

3 participants