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 Overflow in download analysis metadata #3159

Merged

Conversation

antgonza
Copy link
Member

@antgonza antgonza commented Nov 5, 2021

No description provided.

@antgonza antgonza changed the title fix Overflow in download analysis metadata WIP: fix Overflow in download analysis metadata Nov 5, 2021
@coveralls
Copy link

coveralls commented Nov 5, 2021

Coverage Status

coverage: 91.079% (+0.008%) from 91.071%
when pulling 3635b06 on antgonza:fix-Overflow-download-analysis-metadata
into b648221 on qiita-spots:dev.

@antgonza antgonza changed the title WIP: fix Overflow in download analysis metadata fix Overflow in download analysis metadata Nov 5, 2021
@antgonza antgonza requested a review from wasade November 5, 2021 13:37
finally:
del chunk
# pause the coroutine so other handlers can run
await gen.sleep(0.000000001) # 1 nanosecond
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be taken care of by the await on 84?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nop, in fact, if you check the code in the reference, the magic actually happens here; without this everything goes 🍌 ... at least based on that link.

Copy link
Contributor

Choose a reason for hiding this comment

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

for the large data case, it shouldn't be necessary, but since this code is used on small targets it seems reasonable. it's ashame the await None doesn't work. it's possible yield None does but it may depend on aspects of decoration that i dont understand

if mf_fp is not None:
df = qdb.metadata_template.util.load_template_to_dataframe(
mf_fp, index='#SampleID')
response = df.to_dict(orient='index')
response = dumps(df.to_dict(orient='index'))
Copy link
Contributor

Choose a reason for hiding this comment

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

i think all of this makes sense, but I think it should be possible to do this outside of the TRN context, and close the transaction?

await self.flush()
except StreamClosedError:
break
finally:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intention here is for this to be else?

Copy link
Member Author

Choose a reason for hiding this comment

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

I literally borrow this code from the link in the notes ...

Copy link
Contributor

Choose a reason for hiding this comment

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

sure but it doesn't seem necessary here as this method essentialy returns right after StreamClosedError is caught. i don't really care though, so up to you

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I guess the difference is how the original code is creating the chunks vs. how it's done here ... I'll remove.

@antgonza
Copy link
Member Author

antgonza commented Nov 9, 2021

@wasade, finally passing; could you take another look? Thank you.

@wasade wasade merged commit bfacd73 into qiita-spots:dev Nov 9, 2021
@antgonza
Copy link
Member Author

antgonza commented Nov 9, 2021

Thanks @wasade !

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.

None yet

3 participants