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

Scope SQLA session so object lifespans rendering for task history #3058

Closed
wants to merge 1 commit into from
Closed

Conversation

bsdz
Copy link
Contributor

@bsdz bsdz commented Apr 5, 2021

Description

For SQLAlchemy >1.4 it seems setting expire_on_commit to False is less forgiving that older versions. This causes the following exception to be thrown when visiting the "/history" tab.

sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <TaskRecord at 0x7f943e167190> is not bound to a Session; lazy load operation of attribute 'parameters' cannot proceed (Background on this error at: http://sqlalche.me/e/14/bhk3)

Motivation and Context

This resolves issue #3006.

Have you tested this? If so, how?

I ran my jobs with this code and it works for me when using SQLAlchemy 1.4.1.

@bsdz bsdz requested review from dlstadther, Tarrasch and a team as code owners April 5, 2021 21:02
@robsalasco
Copy link

would be nice if this patch can be merged in the next version 😞

@tashrifbillah
Copy link
Contributor

I am giving this PR a go. The current release 3.0.3 did not show /history for me in my CentOS 7 Linux machine. After manually affecting these changes, now I can see /history. Thanks for the work @bsdz .

cc @dstandish

@pmhaddad
Copy link

pmhaddad commented Aug 5, 2021

Commenting just to make it easier to keep an eye on this PR and related Issue, since we are also being affected by this at the moment 👍

@robsalasco
Copy link

ping @Tarrasch

@tashrifbillah
Copy link
Contributor

ping @Tarrasch

He is not the maintainer anymore ...

@dlstadther
Copy link
Collaborator

I'm good to merge this if we can get the feature branch updated with master and tests passing

@tashrifbillah
Copy link
Contributor

Noting the current working combination until this is merged--pip install luigi==2.8.12 sqlalchemy==1.3.16

@tashrifbillah
Copy link
Contributor

I'm good to merge this if we can get the feature branch updated with master and tests passing

ping @bsdz

@bsdz
Copy link
Contributor Author

bsdz commented Aug 19, 2021

I'm good to merge this if we can get the feature branch updated with master and tests passing

ping @bsdz

Very sorry I'm no longer using this project and haven't time to update this fork.

@tashrifbillah
Copy link
Contributor

Maybe another of us can volunteer to update his fork ...

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.

@dlstadther
Copy link
Collaborator

Replaced by #3153

@dlstadther dlstadther closed this Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants