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

datastream: optimize memory usage on ORCID sync #439

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

jrcastro2
Copy link
Contributor

No description provided.

Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

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

LGTM, nice catches overall :)

Only one minor comment on the dict iteration part.

invenio_vocabularies/contrib/names/datastreams.py Outdated Show resolved Hide resolved
invenio_vocabularies/cli.py Show resolved Hide resolved
@jrcastro2 jrcastro2 force-pushed the improve-orcid-sync branch 2 times, most recently from dec7ae4 to 05b5d79 Compare November 22, 2024 15:48
yield result
}

for orcid, future in list(futures.items()):
Copy link
Contributor

Choose a reason for hiding this comment

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

is casting to list needed?

Copy link
Member

@slint slint Nov 25, 2024

Choose a reason for hiding this comment

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

Could also be just iterating of list(futures.keys()), since the reason is that we're modifying the futures dictionary as we iterate over the keys. So "consuming" the iterator iterator is necessary. See also #439 (comment)

@jrcastro2 jrcastro2 self-assigned this Nov 25, 2024
@jrcastro2 jrcastro2 force-pushed the improve-orcid-sync branch 2 times, most recently from 0e7dae6 to b60d386 Compare December 6, 2024 10:27
@jrcastro2 jrcastro2 force-pushed the improve-orcid-sync branch 3 times, most recently from 947daf4 to 375c51d Compare December 9, 2024 07:50
@jrcastro2 jrcastro2 merged commit bb76080 into inveniosoftware:master Dec 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants