place: fix case in which PR region does not have constraints

[acomodi]

Signed-off-by: Alessandro Comodi [email protected]

Description

The new partition region code might not take into account that partition regions are not being set and constraints are not initialized. This PR is an attempt at fixing this.

Related Issue

#1711

Types of changes

• Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

[acomodi]

cc @sfkhalid @vaughnbetz

[sfkhalid]

@acomodi During clustering each new cluster block is assigned an empty PartitionRegion to start with, which can then be either populated with constraint regions or not populated with constraint regions. I think this check is a good one to have, but I am not sure if it is the cause of the issue

[acomodi]

@sfkhalid Ok, I'd need to investigate a bit more and find the source of the issue. The question I am trying to answer is also why this behaviour was not caught previously by CI, so either some change not related to the Partition Regions might have indirectly caused the root issue that leads to this failure.

[vaughnbetz]

I don't think we want the new code. cluster_constraints is stored as a vector indexed by cluster block number. So this change makes the code longer and I think takes us from an O(1) lookup to an O(n) search.

[vaughnbetz]

I looked into the .keys() implementation. The code change would definitely take us from O(1) to O(N) in code that can be performance critical. We should not make this change.

[acomodi]

@vaughnbetz Sure, I will change the fix. As it turns out, there is some other kind of problem preventing the Nightly build to complete successfully.

[vaughnbetz]

@acomodi, @sfkhalid : I think this fix is no longer needed, as Sarah now has put back code to ensure the original function (which checked if you had a partition region on a block) works in all cases / flows. @sfkhalid : can you confirm that the code is now safe (will return false) for blocks with no partition region? If so, we can close this PR and not put back the code (which looks slow).

[sfkhalid]

@acomodi, @sfkhalid : I think this fix is no longer needed, as Sarah now has put back code to ensure the original function (which checked if you had a partition region on a block) works in all cases / flows. @sfkhalid : can you confirm that the code is now safe (will return false) for blocks with no partition region? If so, we can close this PR and not put back the code (which looks slow).

Yes, this fix is no longer needed. The function returns false for blocks with an empty partition region. This will work in any flow because there is a routine now that assigns each cluster a partition region (empty or otherwise if it has constraints) in the case where the pack stage is loaded rather than performed.

[vaughnbetz]

Closing this PR as unneeded.