Skip to content

[cfggen] Remove NatSorted#5601

Merged
tahmed-dev merged 1 commit intosonic-net:201911-t0from
tahmed-dev:taahme/sonic-cfggen-remove-natsorted
Oct 14, 2020
Merged

[cfggen] Remove NatSorted#5601
tahmed-dev merged 1 commit intosonic-net:201911-t0from
tahmed-dev:taahme/sonic-cfggen-remove-natsorted

Conversation

@tahmed-dev
Copy link
Contributor

Natural sorting of SONiC config gen output consumes lot of CPU cycles.
The sole use of natsorted was to make test comparison easier and so,
the natsorting logic is now relocated to the test suite. As a result
sonic-cfggen gained nearly 1 sec per call since we no longer import
natsorted module!

singed-off-by: Tamer Ahmed [email protected]

@tahmed-dev tahmed-dev requested a review from abdosi October 12, 2020 21:53
@lguohan
Copy link
Collaborator

lguohan commented Oct 13, 2020

good improvement. is there a way to make sure the natsorted is not introduced again in the future accidentally? it is easier missed. especially, I am a little concern that if one of the dependency packages that sonic-cfggen uses will introduce natsorted. this is not a real concern for this pr. it is about future-proofing.

@tahmed-dev tahmed-dev force-pushed the taahme/sonic-cfggen-remove-natsorted branch from 2547f4b to 26d5c1a Compare October 13, 2020 18:01
@tahmed-dev
Copy link
Contributor Author

good improvement. is there a way to make sure the natsorted is not introduced again in the future accidentally? it is easier missed. especially, I am a little concern that if one of the dependency packages that sonic-cfggen uses will introduce natsorted. this is not a real concern for this pr. it is about future-proofing.

Thanks! I am aware of putting a package on hold using apt sudo apt-mark hold <package-name> not sure about pip though. @jleveque is there something similart in pip? Or can we whitelist packages?

@jleveque
Copy link
Contributor

jleveque commented Oct 13, 2020

We use the natsort package in other packages like sonic-utilities and sonic-py-common, so we need to continue installing it in the host OS. I don't think there's any way we can prevent it from being re-introduced to sonic-cfggen other than being attentive to code reviews. Maybe add documentation somewhere?

Natural sorting of SONiC config gen output consumes lot of CPU cycles.
The sole use of natsorted was to make test comparison easier and so,
the natsorting logic is now relocated to the test suite. As a result
sonic-cfggen gained nearly 1 sec per call since we no longer import
natsorted module!

singed-off-by: Tamer Ahmed <[email protected]>
@tahmed-dev tahmed-dev force-pushed the taahme/sonic-cfggen-remove-natsorted branch from 26d5c1a to 975cf35 Compare October 13, 2020 23:28
@tahmed-dev tahmed-dev merged commit fdf423e into sonic-net:201911-t0 Oct 14, 2020
lguohan pushed a commit that referenced this pull request Oct 30, 2020
Natural sorting of SONiC config gen output consumes lot of CPU cycles.
The sole use of natsorted was to make test comparison easier and so,
the natsorting logic is now relocated to the test suite. As a result
sonic-cfggen gained nearly 1 sec per call since we no longer import
natsorted module!

singed-off-by: Tamer Ahmed <[email protected]>
abdosi pushed a commit that referenced this pull request Dec 4, 2020
Natural sorting of SONiC config gen output consumes lot of CPU cycles.
The sole use of natsorted was to make test comparison easier and so,
the natsorting logic is now relocated to the test suite. As a result
sonic-cfggen gained nearly 1 sec per call since we no longer import
natsorted module!

singed-off-by: Tamer Ahmed <[email protected]>
abdosi pushed a commit that referenced this pull request Dec 22, 2020
Natural sorting of SONiC config gen output consumes lot of CPU cycles.
The sole use of natsorted was to make test comparison easier and so,
the natsorting logic is now relocated to the test suite. As a result
sonic-cfggen gained nearly 1 sec per call since we no longer import
natsorted module!

singed-off-by: Tamer Ahmed <[email protected]>
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.

3 participants