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

Ensure default Set values are sorted for a reproducible build #535

Conversation

lamby
Copy link
Contributor

@lamby lamby commented Oct 14, 2019

Whilst working on the Reproducible Builds effort we noticed that traitlets generates non-reproducible output which is affecting the reproducibility of other packages. For example, in nbconvert:

  -<dd><p class="first">Default: {'image/jpeg',</span> <span class="pre">'image/svg+xml',</span> [..]
  +<dd><p class="first">Default: {'image/svg+xml',</span> <span class="pre">'application/pdf',</span> [..]

(From https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/nbconvert.html as of time-of writing.)

This is due to it not iterating over a Set traitlet type in a deterministic ordering when generating the Default: [..] human-readable strings. This patch ensures that we display the Set traits defaults in a sorted, and thus reproducible, manner.

@Carreau Carreau force-pushed the 942342-traitlets-please-make-the-output-reproducible branch from f7e4d00 to d0213fc Compare May 31, 2020 05:33
@Carreau
Copy link
Member

Carreau commented May 31, 2020

(rebased to update with master and re-kick the tests)

@Carreau
Copy link
Member

Carreau commented Jun 1, 2020

That actually make the default a a list instead of a set, which is technically wrong. That should be fix in the representation instead of the data structure itself, it looks like this can be done by definind "default_value_repr".

And yes having reproducible build would be good.

@lamby lamby force-pushed the 942342-traitlets-please-make-the-output-reproducible branch from d0213fc to 2e33355 Compare June 2, 2020 14:17
@lamby
Copy link
Contributor Author

lamby commented Jun 2, 2020

Ah, hm, I can't remember if that was deliberate - when glancing at the code many months on I seem to recall that there was something that made me do it this way, but it really could have been that I had not seen this method. Hmm. :)

@Carreau
Copy link
Member

Carreau commented Jun 2, 2020

No problem, I haven't looked at traitlets in a long time and will need to dive again into the codebase to understand what have changed.

THanks again for your patience, and sorry if it take some time to get things back in shape.

Copy link
Collaborator

@rmorshea rmorshea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

lamby and others added 2 commits August 11, 2020 11:07
Whilst working on the Reproducible Builds effort [0] we noticed
that traitlets generates non-reproducible output which is affecting
the reproducibility of other packages. For example, in nbconvert:

  -<dd><p class="first">Default: {'image/jpeg',</span> <span class="pre">'image/svg+xml',</span> [..]
  +<dd><p class="first">Default: {'image/svg+xml',</span> <span class="pre">'application/pdf',</span> [..]

(From https://tests.reproducible-builds.org/debian/rb-pkg/unstable/
amd64/nbconvert.html on 20191014)

This is due to it not iterating over a Set traitlet type in a
deterministic ordering when generating the "Default:" human-readable
string.

This patch ensures that we display the Set traits defaults in a
sorted, and thus reproducible, manner.

  [0] https://reproducible-builds.org/
@Carreau Carreau force-pushed the 942342-traitlets-please-make-the-output-reproducible branch from 2e33355 to df72b28 Compare August 11, 2020 18:18
@Carreau
Copy link
Member

Carreau commented Aug 11, 2020

rebased and should have fixed the test. I tweaked the repr to be set(), for empty set and {1,2,3} for set, but ordered.

@Carreau Carreau added this to the 5.0 milestone Aug 11, 2020
@Carreau
Copy link
Member

Carreau commented Aug 11, 2020

Aaaand it works.

@Carreau Carreau merged commit b0cf372 into ipython:master Aug 11, 2020
4 checks passed
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.

None yet

3 participants