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

Sort keys in dicts in output yaml for 'config' command #1049

Merged
merged 1 commit into from Jun 9, 2020

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented May 4, 2020

Description:

Having different output each time due to random key order makes some
tasks harder (such as CI scripts). So, let's sort the keys to have same yaml output each time.

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

Having different output each time due to random key order makes some
tasks harder (such as CI scripts).
@glmnet
Copy link
Member

glmnet commented Jun 9, 2020

Sure!

@glmnet glmnet merged commit 64bd33a into esphome:dev Jun 9, 2020
@Mikaeeld Mikaeeld mentioned this pull request Jun 12, 2020
sashao pushed a commit to sashao/esphome that referenced this pull request Jun 16, 2020
Having different output each time due to random key order makes some
tasks harder (such as CI scripts).
natp13 pushed a commit to natp13/esphome that referenced this pull request Jun 17, 2020
Having different output each time due to random key order makes some
tasks harder (such as CI scripts).
natp13 added a commit to natp13/esphome that referenced this pull request Jun 17, 2020
@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
@esphome esphome unlocked this conversation Jul 24, 2020
@OttoWinter
Copy link
Member

@ivan4th Please explain what you mean by "random key order".

ESPHome uses OrderedDicts in config parsing practially everywhere, so the order of the keys should match that of how you entered them. This is also why the values should not be sorted, this way the keys are in the same order as before and it's easier to see when looking over the output.

Plus in Python 3.6 you have have guaranteed insertion order for normal dicts, so even if OrderedDict is not used somewhere that should not be an issue.

@ivan4th
Copy link
Contributor Author

ivan4th commented Jul 24, 2020

@OttoWinter The observed behaviour at the time of this PR was indeed this: each time I run esphome with the same input, I get different key order in the output. Now I suspect this could be related to this PyYAML bug, though, so as it turns out sorted(...) was a workaround rather than the real fix. Back then requirements.txt had PyYAML==5.2 so it is possible that the bug was still present (now esphome uses the latest PyYAML version). I will re-check fresh esphome for this in my env shortly, and if this is the case, we should indeed revert this commit as the workaround is no longer needed.

@OttoWinter
Copy link
Member

OttoWinter commented Jul 24, 2020

Ok you appear to be right:

$ for i in {1..4}; do diff -u <(esphome tests/test$i.yaml config) <(esphome tests/test$i.yaml config); done
--- /proc/self/fd/12	2020-07-24 22:41:42.194354945 +0200
+++ /proc/self/fd/14	2020-07-24 22:41:42.194354945 +0200
@@ -27,10 +27,10 @@
         blue: 0.0
         white: 1.0
   build_path: build/test1
-  libraries: []
-  arduino_version: espressif32@1.12.1
   includes: []
+  libraries: []
   platformio_options: {}
+  arduino_version: espressif32@1.12.1
# ...

@ivan4th
Copy link
Contributor Author

ivan4th commented Jul 24, 2020

Actually, seems like the most recent dev commit 43d5e7a appears to still have this problem, so new PyYAML didn't help after all :( So either it is not completely fixed in PyYAML or there's another issue in esphome. I just tried reverting my commit and got the problem of varying output again. So, while sorting keys may not be the proper fix (if we want to keep user's key order), reverting the commit is not correct thing to do either (as random key order doesn't add any value), so we need to understand the cause of this problem. It's already late here, so I'll try to look into thi on weekend.

Quick test with docker to make sure my env doesn't affect things:

ivan4th@i4mbp:~$ docker run -it --rm --entrypoint /bin/bash python:3.8.5-buster
root@9552238c916c:/# git clone -b dev https://github.com/esphome/esphome.git
Cloning into 'esphome'...
remote: Enumerating objects: 100, done.
remote: Counting objects: 100% (100/100), done.
remote: Compressing objects: 100% (76/76), done.
remote: Total 14226 (delta 50), reused 52 (delta 24), pack-reused 14126
Receiving objects: 100% (14226/14226), 5.55 MiB | 1.64 MiB/s, done.
Resolving deltas: 100% (10097/10097), done.
root@9552238c916c:/# cd esphome/
root@9552238c916c:/esphome# git log -1
commit 43d5e7a8ccdba47edfa65f64c34bfd889c918e31 (HEAD -> dev, origin/dev, origin/HEAD)
Author: Otto Winter <otto@otto-winter.com>
Date:   Fri Jul 24 22:37:19 2020 +0200

    Fix WLED minor issues (#1193)
root@9552238c916c:/esphome# ls
CODE_OF_CONDUCT.md  README.md       pylintrc               script
CONTRIBUTING.md     docker          pytest.ini             setup.cfg
LICENSE             esphome         requirements.txt       setup.py
MANIFEST.in         platformio.ini  requirements_test.txt  tests
root@9552238c916c:/esphome# pip install -r requirements.txt

...

root@9552238c916c:/esphome# python setup.py install

...

root@9552238c916c:/esphome# cat >a.yaml
api:
  reboot_timeout: 0s
captive_portal: {}
esphome:
  board: esp32doit-devkit-v1
  name: weather
  platform: esp32
logger: {}
mqtt:
  broker: 10.0.0.1
  discovery: true
ota: {}
wifi:
  ap:
    password: notpskatall
    ssid: Nonexistent_Hotspot
  password: notpasswordatall
  ssid: Nonexistent_WiFi
root@9552238c916c:/esphome# esphome a.yaml config >/tmp/a
INFO Reading configuration a.yaml...
INFO Configuration is valid!
root@9552238c916c:/esphome# esphome a.yaml config >/tmp/b
INFO Reading configuration a.yaml...
INFO Configuration is valid!
root@9552238c916c:/esphome# diff /tmp/a /tmp/b

# NOTE: No differences here!!!

root@9552238c916c:/esphome# git revert --no-commit 64bd33a94eb723ea882d4afa36360780fea3cfb8
root@9552238c916c:/esphome# python setup.py install

...

root@9552238c916c:/esphome# esphome a.yaml config >/tmp/a
INFO Reading configuration a.yaml...
INFO Configuration is valid!
root@9552238c916c:/esphome# esphome a.yaml config >/tmp/b
INFO Reading configuration a.yaml...
INFO Configuration is valid!
root@9552238c916c:/esphome# diff /tmp/a /tmp/b
5a6
>   platformio_options: {}
7d7
<   arduino_version: espressif32@1.12.1
9c9
<   platformio_options: {}
---
>   arduino_version: espressif32@1.12.1
16c16,17
<   baud_rate: 115200
---
>   logs: {}
>   level: DEBUG
17a19
>   baud_rate: 115200
19,20d20
<   level: DEBUG
<   logs: {}
24,28d23
<   reboot_timeout: 15min
<   topic_prefix: weather
<   username: ''
<   keepalive: 15s
<   discovery_prefix: homeassistant
29a25
>   username: ''
30a27
>   discovery_prefix: homeassistant
31a29,31
>   keepalive: 15s
>   topic_prefix: weather
>   reboot_timeout: 15min
52d51
<   safe_mode: true
54a54
>   safe_mode: true
60d59
<   reboot_timeout: 15min
63a63
>   reboot_timeout: 15min
root@9552238c916c:/esphome#

@OttoWinter
Copy link
Member

Ok, I found the issues (neither directly related to PyYAML) and fixed them here: 99fe519 (#1191)

  • First: the ensure_list validator for some reason doesn't preserve the order. I'm not 100% sure why but I know it's due to voluptuous somehow. I replaced it with a simpler method and that works fine.
  • The voluptuous schema default keys in schema_builder.py are looked up using a set(), but python doesn't guarantee insertion order for those. I updated it to use a list instead.

Now the test script I wrote above works fine.

@ivan4th
Copy link
Contributor Author

ivan4th commented Jul 24, 2020

@OttoWinter yes, this indeed works fine, thanks! 👍

This was referenced Jul 26, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants