1370197 - cross-channel-l10n: sparse graph implementation

Status: RESOLVED FIXED

(Developer Services :: General, task)

Description

[Axel Hecht [:Pike]]

We'll need a sparse graph for cross-channel l10n, and it's the part of my experimental code that's most clearly defined and ready for review.

Splitting this out into its own bug.

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1370197, sparse graph for mercurial commits, in support of cross-channel l10n,

Review commit: https://reviewboard.mozilla.org/r/160462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160462/

Comment 2

[Axel Hecht [:Pike]]

Attachment: bug 1370197, add documentation for cross-channel l10n, in particular sparse graphs,

Review commit: https://reviewboard.mozilla.org/r/160464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160464/

Comment 3

[Francesco Lodolo [:flod]]

Comment on attachment 8889423 [details]
bug 1370197, add documentation for cross-channel l10n, in particular sparse graphs,

https://reviewboard.mozilla.org/r/160464/#review165730

Some small edits on the text.

::: docs/cross-channel-l10n/architecture.rst:21
(Diff revision 1)
> +#. Updating content per push of original repository
> +#. Updating content for configuration changes
> +
> +The intent is that the initial content creation is actually the same as
> +a configuration change that adds new files. One could choose to do otherwise,
> +but this is also a good way to ensure that the change handling configuration

Looks like "change" is unneeded?

::: docs/cross-channel-l10n/graph.rst:55
(Diff revision 1)
> +    "green2" -> "green3" ;
> +    "green3" -> "green4" ;
> +    "green4" -> "green5" ;
> +    "green4" -> "green4.5" ;
> +
> +Natively, mercurial creates a graph that shows all paths from each node

nit: Mercurial

::: docs/cross-channel-l10n/index.rst:57
(Diff revision 1)
> +
> +  open-window = Open Window
> +  open-new-window = Open New Window
> +  close-window = Close Window
> +
> +The generated repository should have two files, which paths correspond to

which - whose?

::: docs/cross-channel-l10n/index.rst:59
(Diff revision 1)
> +  open-new-window = Open New Window
> +  close-window = Close Window
> +
> +The generated repository should have two files, which paths correspond to
> +those in the upstream repositories, but don't have the ``locales/en-US`` in
> +them. The contents of :file:`browser/browser.properties` are

contents…are -> content…is

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8889422 [details]
bug 1370197, sparse graph for mercurial commits, in support of cross-channel l10n,

https://reviewboard.mozilla.org/r/160462/#review165874

Consider this a rubber stamp r+ rather than a full review.

I'm still not convinced that this approach towards tackling l10n needs is best. It feels very much like a "if all you have is a hammer everything looks like a nail" type scenario, where the hammer in this case is a version control tool. I suspect we could accomplish something similar to DAG walking by iterating through the pushlog of various repos and e.g. taking push heads from various repositories, analyzing files within, and stuffing the results in version control.

But, the approach in this patch isn't wrong per se.

My algorithms and graph walking skills aren't terrific. That's the main reason I can't give an adequate review here. The type of graph traversal code being written is a bit fragile when it comes to bugs and performance scaling. I'm sure if I squinted hard enough I could find some "accidentally quadratic" bugs that could lead to very long run times against the Firefox repo.

Anyway, I don't want to hinder your progress on l10n matters. So please take my r+ on this patch as a blessing to continue with whatever comes next.

::: cross-channel-l10n/mozxchannel/graph.py:73
(Diff revision 1)
> +        del self.archived_parents[:]
> +        del self.merges[:]

`self.archived_parents.clear()` is a bit clearer IMO.

::: cross-channel-l10n/mozxchannel/graph.py:138
(Diff revision 1)
> +
> +    def eliminateShortCuts(self):
> +        '''Go through the graph and remove shortcut circles.
> +        '''
> +        major_childs = sorted(self.merges, key=lambda c: len(self.parents[c]))
> +        for CHILD in major_childs:

The use of an `UPPERCASE` variable here is really wonky. Typicall `UPPERCASE` variables are global constants.

::: cross-channel-l10n/prod-requirements.txt:1
(Diff revision 1)
> +Mercurial==4.1.2 \
> +    --hash=sha256:09415253fb409a77e19b9951532a3c22c4e07e74bab80652527064617daab194

Mercurial 4.2.2 is current stable and 4.3 will be released very soon now. It is best to stick with the latest version.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8889423 [details]
bug 1370197, add documentation for cross-channel l10n, in particular sparse graphs,

https://reviewboard.mozilla.org/r/160464/#review168530

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 8889422 [details]
bug 1370197, sparse graph for mercurial commits, in support of cross-channel l10n,

https://reviewboard.mozilla.org/r/160462/#review165874

> `self.archived_parents.clear()` is a bit clearer IMO.

.clear on list was only added in python 3.3. I need py2 compat for a while still.

Which triggers a related question, is there a python 2 vs 3 policy in vct?

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 8889422 [details]
bug 1370197, sparse graph for mercurial commits, in support of cross-channel l10n,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160462/diff/1-2/

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8889423 [details]
bug 1370197, add documentation for cross-channel l10n, in particular sparse graphs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160464/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b9d7fe52654b
sparse graph for mercurial commits, in support of cross-channel l10n, r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/abb53b78c209
add documentation for cross-channel l10n, in particular sparse graphs, r=gps

Comment 10

[Axel Hecht [:Pike]]

Thanks.

Re the algorithm concerns, I tested a plentitude of algorithms to have less data to walk through. Sadly, all the ones I tried ended up creating worse output and were equally complex. It is kinda weird, but just going through most of things in the way that they were initially created is both the simplest way to do it, and the one leading to the best results in the output.