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

Optimize the category PK search #104

Closed
martonmiklos opened this issue Nov 1, 2022 · 7 comments · Fixed by #122
Closed

Optimize the category PK search #104

martonmiklos opened this issue Nov 1, 2022 · 7 comments · Fixed by #122
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@martonmiklos
Copy link
Contributor

Hey,

I have recently added the name field of the Categories to the filterable columns in InvenTree:
inventree/InvenTree#3854

This would allow to make this code to be made more efficient:

for item in part_categories:

with simplifying to:

    # Fetch all categories
    part_categories = PartCategory.list(inventree_api, name=category_name)
    if len(part_categories) == 1:
        return part_categories[0].pk

    return -1

This change got introduced with the InvenTree API version 78, the inventree-python depends on the version 51:
https://github.com/inventree/inventree-python/blob/master/inventree/api.py#L25

It might be possible to add a condition based on the server's API version?

@eeintech
Copy link
Contributor

eeintech commented Nov 2, 2022

@martonmiklos Where is the gain in your experience and by how much? Adding conditions based on API version is going to be tough to maintain... it would be preferable to request the InvenTree Python API to catch up and up the Ki-nTree dependency. What do you think?

@martonmiklos
Copy link
Contributor Author

Where is the gain in your experience and by how much?
Well I imported my category tree from the TME and it consists ~1700 categories.

What do you think?

Increasing the min API version in the python API would mean that the python API would drop support for older InvenTree versions which I think until a point when it is not necessary should be avoidable.

I do not know how wide is the Ki-n-tree user base, but the best possible option would be setting an API barrier here, and throw error if lower API version of InvenTree is connected.

@eeintech
Copy link
Contributor

eeintech commented Nov 3, 2022

The thing is, I'm not really sure where the gain is... I don't have any performance issue using Ki-nTree.
So I don't find value in adding an unnecessary constraint, unless it achieves really great rewards.

@martonmiklos
Copy link
Contributor Author

Just to take some numbers to the desk I put together the following script:
https://gist.github.com/martonmiklos/20df2a75de77cee06d6dc755e65bdb0e

It gives me the following run time (with 1618 categories):

(env) mm@lapos:/tmp$ python3 benchmark.py 
test category name match
Name match only: 330.321550 ms 
test category name match
Name and parent pk match: 351.499796 ms 
Filter using InvenTree: 33.458471 ms 

I have not traced the Kin-Tree yet how often does this category filter function get called, but at some point I would like to add mass import feature from CSV to it where the 30 vs. 300 ms ish time could count something.

My recommendation would be to keep this in mind and at some point (maybe when the InvenTree python API sets the min. level above the necessary) we can drop the current code.

@eeintech
Copy link
Contributor

eeintech commented Nov 4, 2022

Ok that's a nice improvement and yes I agree with the strategy of waiting till older versions of InvenTree become really old to assume we can make the change.

It is related to the implementation you did in #87 right?

@martonmiklos
Copy link
Contributor Author

It is related to the implementation you did in #87 right?

Well I have been worked on an Arena PLM migration tool where implementing it became necessary to implement it in InvenTree.
I just peeked the code during the #87 and I came across it came into my mind.
But it does not necessary to implement my plans in #87.

@eeintech
Copy link
Contributor

eeintech commented Nov 9, 2022

Thanks @martonmiklos. Currently on InvenTree's 0.8.x branch the latest API version is 69, quite a bit behind! (no pun intended)

I would guess that for the imminent 0.9.0 release the API version will jump to 80 or later. If that's the case, I will update the get_inventree_category_id method with your proposed change and note it in the release notes so people are aware it could be a breaking change if they don't upgrade InvenTree to latest version.

Sounds reasonable?

@eeintech eeintech added the enhancement New feature or request label Nov 9, 2022
@eeintech eeintech added this to the 1.0.0 milestone Feb 16, 2023
@eeintech eeintech mentioned this issue Feb 20, 2023
6 tasks
@eeintech eeintech self-assigned this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants