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(xmlupload): fix performance slowdown during stash applying (DEV-2717) #533

Conversation

Nora-Olivia-Ammann
Copy link
Collaborator

No description provided.

@linear
Copy link

linear bot commented Sep 28, 2023

DEV-2717 xml_upload_stash: improve "_replace_internal_ids_with_iris" function

The function _replace_internal_ids_with_iris function in the file xml_upload_stash.py is slowing down the entire xmlupload. The function iterates over an entire dictionary to find the id to replace. The bigger the dictionary, the longer the function takes.

in a file with 10 links this replace function takes 415ns.

In a file with 1000 links this function takes 278ms 420ns. This is longer than by a factor of nearly 670'000.

1000r.png

This slow dictionary function will be replaced.

BaseError is built differently from KeyError. Therefore how the message has to be retrieved is different too.
@Nora-Olivia-Ammann Nora-Olivia-Ammann changed the title refactor(xmlupload): improve-_replace_internal_ids_with_iris function (DEV-2717) refactor(xmlupload): improve _replace_internal_ids_with_iris function (DEV-2717) Sep 28, 2023
New methods to extract and replace string in KnoraStandoffXml
Rewrite _replace_internal_ids_with_iris method
Fix docstring
remove separate method to replace (testing should be with the class)
The IRI prefix is only if it is an internal id
mypy complained that the unittest (next commit) were not typed
make function private
Add unittest for new replace function
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Overall looks good. I added some minor remarks

test/unittests/test_xml_upload_stash.py Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
test/unittests/test_xml_upload_stash.py Show resolved Hide resolved
src/dsp_tools/models/value.py Outdated Show resolved Hide resolved
test/unittests/test_xml_upload_stash.py Show resolved Hide resolved
test/unittests/test_xml_upload_stash.py Outdated Show resolved Hide resolved
test/unittests/test_xml_upload_stash.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Show resolved Hide resolved
src/dsp_tools/utils/xml_upload_stash.py Outdated Show resolved Hide resolved
@Nora-Olivia-Ammann Nora-Olivia-Ammann changed the title refactor(xmlupload): improve _replace_internal_ids_with_iris function (DEV-2717) fix(xmlupload): fix performance slowdown during stash applying (DEV-2717) Oct 2, 2023
poetry.lock Outdated Show resolved Hide resolved
@jnussbaum
Copy link
Collaborator

@Nora-Olivia-Ammann I just realized that there was already a Linear ticket for this: DEV-2667. Once your PR is merged, could you close that ticket with a comment that it was resolved by this PR?

And could you also close the ticket DEV-1981 with a comment that it was probably resolved by this PR? (But only after this PR is merged)

@Nora-Olivia-Ammann Nora-Olivia-Ammann merged commit add40b2 into main Oct 2, 2023
4 checks passed
@Nora-Olivia-Ammann Nora-Olivia-Ammann deleted the wip/dev-2717-xml_upload_stash-improve-_replace_internal_ids_with_iris-function branch October 2, 2023 15:47
@daschbot daschbot mentioned this pull request Oct 2, 2023
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