Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-10-06
08:48:46 opendevreview Julien LE JEUNE proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156
09:37:23 opendevreview Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156
10:30:00 sean-k-mooney gibi: i dont know if you read this comment https://review.opendev.org/c/openstack/placement/+/962776/comment/f68ed8e5_0803b616/ but when i was running the tests locally i noticed that changing the request_limit paramter had no perforncem imporment
10:30:04 sean-k-mooney """ min(conf.max_allocation_candidates, requst_limit) """
10:30:15 sean-k-mooney i think we are missing ^ somewhere
11:16:26 gibi_ sean-k-mooney: we intentionally not changed the meaning of the limit parameter when max_allocation_candidates was introduced due to
11:16:29 gibi_ Placement generates these candidates fully before applying the limit parameter provided in the allocation candidate query to be able do a random sampling if [placement]randomize_allocation_candidates is True.
11:16:52 gibi_ the limit parameter is applied last
11:17:01 gibi_ if we change that then that changes how randomize_allocation_candidates behave
11:17:10 gibi_ so we did not changed limit
11:35:43 sean-k-mooney hum i see
11:35:48 sean-k-mooney that is unfortunate
11:36:04 sean-k-mooney but i guess that the legacy we are stuck with
11:37:03 sean-k-mooney there are two cases i guess to consdier request_limit < max_allocation_candidates
11:37:31 sean-k-mooney i was hoping in this case we coudl treadt it as if max_allocation_candidates == request_limit
11:37:55 sean-k-mooney in the other case its already going to be enforced
11:39:09 sean-k-mooney i wonder if we coudl implemetn the randomize_allocation_candiates diffently
11:39:24 sean-k-mooney we currently have depthfirst or breathfirst search
11:39:48 sean-k-mooney but couldl we instead do a random search for that usecase.
11:40:49 sean-k-mooney it would not be exactuly the same behavior but the randomisation was really there because of the orginial depth first approch to generating candiates
11:41:07 sean-k-mooney if we are using breathfirst its much less required
11:41:49 sean-k-mooney that a sepreate poblem then the one that your trying to fix but i wonder fi we should conitnue to supprot that if we cant apply optimisation based on teh request limit
11:42:51 sean-k-mooney in otherwords i think we could provide randomness in the generation fo the candiate without needign to generate all of them and doing a random choice at the end
11:43:20 sean-k-mooney that would fulfil the same usecase but with a lot less wasted effort
11:43:40 sean-k-mooney maybe another topic for the ptg dicussion?
11:46:27 sean-k-mooney gibi: basically im not sure that genrating all possible candiate is part of the api contract fo that parmater so we shoudl be free to evolve its meaning over tiem to be more effeictent provided it still has the same overall effect
11:57:21 gibi I'm OK to discuss this on the PTG. But for me this is a small issue that limit and max_allocation_candidates has two different meaning today
11:58:44 sean-k-mooney ya its minor but it means the clinet cant contol the perfoamce of the request by asking for less cnadiates in the responce
11:59:15 sean-k-mooney to me it undermines why the request_limit parmater exists in the api in the first place
11:59:36 gibi that parameter from the start only limited the size of the response nothing lese
11:59:39 gibi else
12:00:51 sean-k-mooney sure but i woudl expect it to do more then that as an end user and it prevent nova for example form being smart and say (i knwo this query might generate a lot of candiate so im going to use a lower limit dynicmicly)
12:01:01 sean-k-mooney not that nova has the intelegents to know when it shoudl do that
12:01:20 sean-k-mooney so its not a blocker but i just find it surprising
12:02:06 sean-k-mooney gibi: i noticed you commented on your inlining patches that they dont seam to provide a benifit
12:02:27 sean-k-mooney gibi: do you think that is a dead end or are you plannign to do more expermients in that direction
12:03:22 gibi unfortunately the randomize_allocation_candidates option defined as
12:03:23 gibi If True, when limiting allocation candidate results, the results will be a random sampling of the full result set.
12:03:34 gibi *full result set* :/
12:03:59 gibi sean-k-mooney: the first inlining helps, the second not much
12:04:05 sean-k-mooney right but it does not define what the full result set is
12:04:36 gibi this helps If True, when limiting allocation candidate results, the results will be a random sampling of the full result set.
12:04:39 gibi sorry
12:04:41 gibi wrong buffer
12:04:47 gibi this helps https://review.opendev.org/c/openstack/placement/+/963052/1/placement/tests/functional/test_allocation_candidates.py#265
12:05:08 gibi this does not really help https://review.opendev.org/c/openstack/placement/+/963053/2/placement/tests/functional/test_allocation_candidates.py
12:05:10 sean-k-mooney gibi: so inlining _consolidate_allocation_requests helps but not exceeds_capacity
12:05:14 gibi yep
12:06:30 sean-k-mooney i see form the comment you have been trying to use some profiles to help with this, have you found that useful
12:06:41 gibi I will not trying to inline more, the profiler shows that the time is spent on hashing and eqality checking the building blocks but I don't see easy moves there
12:07:02 gibi I used just the built in cProfiler
12:07:09 gibi as you can see in the commented out code
12:07:22 sean-k-mooney ya i was just wondering if the output was useful
12:07:32 sean-k-mooney i have never used that before hence the question
12:08:04 gibi it tells you what called how much and what time it took so it shows you where your spending your execution time
12:08:43 gibi it showed that consolidate is more expensive than exeeced_capacity
12:08:57 sean-k-mooney ack, i have used flamegraph profilers in other lanaguges in the past just never really used those types of tools much with openstack because they hate eventlets
12:09:13 sean-k-mooney placement fortunetly does not use eventlet so that is not an issue
12:09:22 gibi yeah, I just enabled it in athe _merge_candidate call so no parallism was involved
12:09:33 sean-k-mooney but i tried to use them on nova once on the schduler and they were not really that useful when used as external tools
12:10:19 gibi I'm now providing test coverage for the prune patch and we can decide together when dansmith is up that which inlining patch if any we want to take in
12:10:41 sean-k-mooney how do you feel about this overall approch for the current bug. are you happy that it gives us enough breathing room for the current reproted edgecase to move forward with for now?
12:12:23 gibi I think the prune patch already give us time to re-thing the modeling. I think the inlining of the _consoliodate call is still have a good ROI on code complexity. I would at least merge these two optimization. I will then go and update our backlog space about the problem with the new learning and we can discuss on the PTG. My attached customer case probably closable with only the prune patch in
12:12:43 gibi s/space/spec/
12:12:45 sean-k-mooney im kind of suprised that manual function inlineing has such an effect in a dynmic language like python. what python version were you testing with out of interest?
12:13:15 gibi Python 3.12.10 (main, Apr 9 2025, 04:44:59) [GCC 14.2.0] on linux
12:13:36 sean-k-mooney ack i wonder if the impact is large on say 3.10 or 3.9
12:13:43 gibi note that it wasn't pure inlining, it was actually using the commulative nature of the call to save actual steps
12:14:02 gibi cumulative
12:14:24 sean-k-mooney ah ok i was actully trying to pull up the orginal code to comare whiel we were chatting but i have not fond that yet
12:14:47 sean-k-mooney wel i have not been trying very hard :)
12:15:42 sean-k-mooney oh its in your previous patch
12:15:45 gibi consolidate merges the allocations of the different request groups, with the new product algo this was done on each prefix of those groups. but merging G1 + G2 and G1 + G2 + G3 can be optimized if G1 + G2 is only calculated once not twice
12:15:52 sean-k-mooney that explains why it was not in github https://review.opendev.org/c/openstack/placement/+/962776/5/placement/objects/allocation_candidate.py#850
12:16:34 sean-k-mooney so really your folding the exceeds_capacity logic inter the generation
12:16:45 gibi yes
12:17:09 sean-k-mooney ok ya that make more sense as to why it would have a benifit
12:17:14 gibi because of the result of that check informs what part of the product space can be ignored
12:18:02 gibi i.e. if G1+G2 fails the check then G1+G2+Gx+...Gy will awas fail the check so they can be ignored
12:18:29 sean-k-mooney right so we might be able to not inline the fucntion if we return more info beyond True or False like the atribute that exceed capsity
12:19:51 sean-k-mooney i dont really object to the inlineing DRY is nice but im ok wiht specific code where we have a perfroamce need for it
12:20:24 sean-k-mooney im just wonderign if we can get the benift of a testibale function while also achiving the speed up if we adjust the return value to provdie more context
12:21:52 sean-k-mooney looking at https://github.com/openstack/placement/blob/master/placement/objects/research_context.py#L345
12:22:30 sean-k-mooney the behvior is differnt enough that i dont think it makes sense to reuse exceeds_capacity and modify it as i was suggesting
12:24:41 gibi I will think about making the code cleaner once I have pinned down the behavior with some unit test
12:25:47 sean-k-mooney ack i think we coudl factor https://review.opendev.org/c/openstack/placement/+/963052/1/placement/objects/allocation_candidate.py#746 out into its own helper if we wanted but having its own impleaiton i belive is resonable
13:26:13 opendevreview Julien Le Jeune proposed openstack/nova master: Update start_service() function in test https://review.opendev.org/c/openstack/nova/+/963156
13:34:17 bauzas dansmith: remember when you asked me why my comments were usually having two spaces between the hashtag and the first word ? looks to me this is due to autopep8 which automatically 'fixes' my comments 🤦
13:34:43 dansmith bauzas: I don't, but.. good to know
13:38:37 bauzas still have to confirm, but unless I'm fool, I don't understand why those extra spaces suddently appear on my code while I doublechecked before committing I wasn't having them
13:39:02 bauzas so this is either autopep8 or vscode (was having the same problem before I started using Cursor)
13:39:23 bauzas I was becoming crazy tbh
13:40:12 bauzas I'll do further tests with --no-verify
13:40:21 bauzas so it would skip autopep8
14:03:15 sean-k-mooney bauzas: its from pep8 https://peps.python.org/pep-0008/#inline-comments
14:03:38 sean-k-mooney bauzas: "An inline comment is a comment on the same line as a statement. Inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space."
14:03:55 bauzas I'm not talking of inline statements sorry
14:04:19 sean-k-mooney oh ok. never mind then
14:04:26 bauzas just a whole comment line like # TODO(x) : This is a comment
14:04:57 sean-k-mooney ah well ther eshoudl be 1 space between the # and TODO

Earlier   Later