Skip to content

Fix/lazyload-to-eagerload#822

Merged
abrichr merged 9 commits intoOpenAdaptAI:mainfrom
Animesh404:fix/lazyload-to-eagerload
Jul 5, 2024
Merged

Fix/lazyload-to-eagerload#822
abrichr merged 9 commits intoOpenAdaptAI:mainfrom
Animesh404:fix/lazyload-to-eagerload

Conversation

@Animesh404
Copy link
Copy Markdown
Member

@Animesh404 Animesh404 commented Jul 4, 2024

What kind of change does this PR introduce?

a bugfix for #807

Summary

Added joinedload in all the functions called by get_events in order to eagerload them.

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?
Try running replay via VisualReplayStrategy now it shouldn't throw DetachedInstanceError
python -m openadapt.replay VisualReplayStrategy --instructions "calculate 4*2-1"

Copy link
Copy Markdown
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

Thank you @Animesh404 ! What do you think about modifying _get to accept an eager parameter instead?

@Animesh404
Copy link
Copy Markdown
Member Author

Thank you @Animesh404 ! What do you think about modifying _get to accept an eager parameter instead?

modified in 1feea90

Comment thread openadapt/db/crud.py Outdated
Comment thread openadapt/db/crud.py Outdated
Copy link
Copy Markdown
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

Almost there!

Comment thread openadapt/db/crud.py Outdated
Comment thread openadapt/db/crud.py Outdated
Comment thread openadapt/db/crud.py Outdated
Comment thread openadapt/db/crud.py Outdated
@abrichr
Copy link
Copy Markdown
Member

abrichr commented Jul 5, 2024

Thanks @Animesh404 ! Can you please clarify the use of subqueryload vs joinedload? Why are they both used in different locations?

@Animesh404
Copy link
Copy Markdown
Member Author

Thanks @Animesh404 ! Can you please clarify the use of subqueryload vs joinedload? Why are they both used in different locations?

both are used for eagerloading. subqueryload is used for indirect relationships whereas joinedload for direct ones.

Copy link
Copy Markdown
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

LGTM!

@abrichr abrichr merged commit 615fc6f into OpenAdaptAI:main Jul 5, 2024
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.

2 participants