1035333 - Build Telemetry Experiment for Vi/Pl/Tr translation trial on beta

Status: RESOLVED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Similar to bug 1022411, this is about building the experiment for our beta trial. Besides what was done in bug 1022411, we need to think about:

- Redefining the experiment parameters
- The "Translations by" string is not localized on beta, so the experiment will need to set up the correct string depending on the locale (see bug 1032139). Or maybe Florian is planning to do this directly in the Translations.jsm code instead of through the experiment.. I don't know
- The telemetry-experiments-server has an experiment running limit of 42 days. I think this is arbitrary and we can just raise it, but we need to check with Benjamin about it. I believe the running time for this experiment will be somewhere between 12 - 18 weeks, so more than 42 days.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes from comment #0)
> - The "Translations by" string is not localized on beta, so the experiment
> will need to set up the correct string depending on the locale (see bug
> 1032139). Or maybe Florian is planning to do this directly in the
> Translations.jsm code instead of through the experiment.. I don't know

The answer for this part is: the experiment doesn't need to do anything special. Florian handled all this in-tree in bug 1032139

Comment 2

[Gregory Szorc [:gps]]

I'm taking this [from the desktop iteration cycle].

I have little clue what's going on. I'll track down someone in the know (Felipe?) once I'm finished clearing my review queue of other Firefox bugs.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

From bug 1022411 comment 24, this is the title and description that goes into the localized section of install.rdf, in English:

Name: Automatic Translation
Description: Read more of the web by automatically translating sites written in different languages.

Comment 4

[Gregory Szorc [:gps]]

Translation experts: could we please get the 2 strings in comment #3 translated to vi, po, and tu?

Comment 5

[Gregory Szorc [:gps]]

Attachment: Support per-experiment build rules

Previously, experiments were static .xpi files checked into source
control. We enable support for running a Python script to produce the
final .xpi.

This functionality will be leveraged to produce multiple
experiments/XPIs from an input template.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Add virtualenv and package paths to .hgignore

Comment 7

[Gregory Szorc [:gps]]

Attachment: Python API for creating XPI files

The upcoming localization experiments will have their source committed
to the tree and the XPI will be produced during building. Instead of
invoking `zip` manually, we roll our own minimal zip file creation code.

This code is pretty basic. It was compared against existing Python code
in the add-on SDK for sanity. A subsequent patch will demonstrate this.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Add templates for Aurora translation experiment

To support the Vi, Po, and Tu variations of the translation experiment,
we're going to dynamically generate an XPI based on templates of their
files.

This patch adds templates for the translation experiment. The files were
obtained from the existing translation-de-aurora experiment. Every de
specific item has been stripped and replaced with %%ITEM%% template
markers. A subsequent patch will introduce code for building XPIs from
these templates.

Comment 9

[Gregory Szorc [:gps]]

Attachment: Add templates for Aurora translation experiment

Now with moar manifest.json template.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8454184 [details] [diff] [review]
Add templates for Aurora translation experiment

I just realized we have a Python templating library already hooked up. I was planning to roll my own using str.replace(). But I guess I'll look into using genshi.template. Pull review for now. I may hold off publishing more patches until the entire patch set is done.

Comment 11

[Selim Şumlu]

(In reply to Gregory Szorc [:gps] from comment #4)
> Translation experts: could we please get the 2 strings in comment #3
> translated to vi, po, and tu?

Turkish (tr):
Name: Otomatik Çeviri
Description: Farklı dillerde yazılmış siteleri otomatik olarak çevirerek web'de daha fazla içeriği okuyun.

Comment 12

[Stefan Plewako [:stef]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Translation experts: could we please get the 2 strings in comment #3
> translated to vi, po, and tu?

Polish (pl):
Name: Automatyczne tłumaczenia
Description: Korzystaj w większym stopniu z zasobów sieci, dzięki automatycznym tłumaczeniom stron w innych językach.

Comment 13

[hainp2604]

(In reply to Gregory Szorc [:gps] from comment #4)
> Translation experts: could we please get the 2 strings in comment #3
> translated to vi, po, and tu?

Vietnamese (vi):
Name: Dịch tự động
Description: Đọc nội dung dễ dàng hơn bằng việc tự động dịch các trang sang nhiều ngôn ngữ khác nhau.

Comment 14

[Benjamin Smedberg]

Comment on attachment 8454170 [details] [diff] [review]
Support per-experiment build rules

I very slightly feel like this might be better as a separate method either in build.py or within the Experiment class.

Comment 15

[Benjamin Smedberg]

Comment on attachment 8454171 [details] [diff] [review]
Add virtualenv and package paths to .hgignore

Have I mentioned my dislike for virtualenv when it's not needed...

Comment 16

[Benjamin Smedberg]

Comment on attachment 8454172 [details] [diff] [review]
Python API for creating XPI files

I don't know whether the webops build machines are running python 2.7. Are the unicode_literals critical to this?

Comment 17

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> Comment on attachment 8454172 [details] [diff] [review]
> Python API for creating XPI files
> 
> I don't know whether the webops build machines are running python 2.7. Are
> the unicode_literals critical to this?

Pretty sure our infra is running 2.6 (or at least large parts of it are). unicode_literals work in 2.6.

The unicode_literals aren't critical. I just like to make new Python code I write forward compatible so I'm not creating technical debt.

Comment 18

[Benjamin Smedberg]

If unicode_literals works in 2.6, then we're all good. Certainly don't care about anything prior to that.

Comment 19

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Have I mentioned my dislike for virtualenv when it's not needed...

I humbly challenge that this statement stems from a misunderstanding of the importance of virtualenvs to maintain sanity.

A virtualenv is a mechanism to avoid modifying the system/global Python install. "Make your changes in this one-off thing here [a virtualenv] instead of globally." As you work on more and more Python projects, these cumulative hacks [to the system Python] pile up and start to conflict. This leads to bugs, weird behavior, difficult-to-reproduce run-time environments, etc. Virtualenvs operate as Python environment overlays that isolate all these changes to the system Python install. They protect you from shooting yourself with a bullet fired 2 months ago. This is why I always use virtualenvs and recommend others do too.

As a casual Python developer, Benjamin, you may not see a benefit of virtualenvs. But to someone like me who hacks on a few dozen Python projects, virtualenvs are necessary. I use my knowledge of footgun existence to protect those who don't, which is why I recommend virtualenvs in build instructions.

I hope you found this enlightening. I'd be happy to blog in more detail.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Question: is the sha1 hash of the experiment.xpi file identical across different builds, or can some timestamp from buildtime affect the contents of the file (say, metadata about the inner files) produce a different hash each time?

While you're here, if you're willing to do so, it would be nice if the per-experiment build file would contain an optional parameter that would remove the <em:type> from the install.rdf file, or set its value to 1. In the absence of this optional parameter, type=128 should be preserved. This would be useful to use the build script during development to test the add-on as a normal add-on before producing a type=128 version of it. (128 = experiment)

If you don't want to do it or don't care enough, I'll probably add something like this myself the next time I create an experiment.

Comment 21

[Gregory Szorc [:gps]]

(In reply to :Felipe Gomes from comment #20)
> Question: is the sha1 hash of the experiment.xpi file identical across
> different builds, or can some timestamp from buildtime affect the contents
> of the file (say, metadata about the inner files) produce a different hash
> each time?

I made the custom zip packaging logic explicitly omit mtimes to reduce variance during builds. I haven't verified that zips are actually idempotent.

> While you're here, if you're willing to do so, it would be nice if the
> per-experiment build file would contain an optional parameter that would
> remove the <em:type> from the install.rdf file, or set its value to 1. In
> the absence of this optional parameter, type=128 should be preserved. This
> would be useful to use the build script during development to test the
> add-on as a normal add-on before producing a type=128 version of it. (128 =
> experiment)

Ooh, that's a good idea! I'll probably brush into this when I test this extension. I got side-tracked today by prior Firefox bugs :/

Comment 22

[:Felipe Gomes (needinfo for replies!)]

(In reply to Gregory Szorc [:gps] from comment #21)
> (In reply to :Felipe Gomes from comment #20)
> > Question: is the sha1 hash of the experiment.xpi file identical across
> > different builds, or can some timestamp from buildtime affect the contents
> > of the file (say, metadata about the inner files) produce a different hash
> > each time?
> 
> I made the custom zip packaging logic explicitly omit mtimes to reduce
> variance during builds. I haven't verified that zips are actually idempotent.

This is important, because a hash change is what triggers an update of the experiment, and we don't want to unnecessarily send users through the update path of all experiments whenever a new one is added and the server files are rebuilt.

If it's not possible to do that with certainty, maybe you can change your restriction of "either experiment.xpi or build.py" to "if there's an experiment.xpi file, use it; otherwise build it with the build script". This way we can use the build script to manually build the final xpi, but all the src and build instructions are still checked in.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

There's a new requirement on the behavior of the experiment that differs a bit from the previous one. On installation, every user will get the "detectLanguage" pref activated. But the test/control groups should only be split after 10 days of the experiment running. This is to better measure the baseline data for each group instead of just believing they are similar.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

(In other words, the pref showTranslationUI should only be toggled to true for the test group after 10 wall days of running the experiment).

Comment 25

[Benjamin Smedberg]

The 10-day requirement should be filed and fixed separately. Let's avoid scopecreep!

Comment 26

[:Felipe Gomes (needinfo for replies!)]

I filed bug 1038493 for this. GPS: note that a new template for bootstrap.js might be coming from that bug.

Also, in your manifest.json/install.rdf there are mentions of "aurora", but this experiment is actually going to beta 32.  (e.g. the add-on id should be [e-mail])

Comment 27

[Tracy Walker [:tracy]]

This appears to warrant QA. However, it's not clear what would be required in testing/verification.  Please provide insight, Thank you.

Comment 28

[Gregory Szorc [:gps]]

I was testing dynamic xpi generation yesterday and confirmed the process can be idempotent. i.e. file mtimes aren't leaking into the zip.

Comment 29

[Gregory Szorc [:gps]]

Attachment: Add templates for translation experiment

To support the Vi, Po, and Tu variations of the translation experiment,
we're going to dynamically generate an XPI based on templates of their
files.

This patch adds templates for the translation experiment. The files were
obtained from the existing translation-de-aurora experiment. Every de
specific item has been stripped and replaced with template variables.
A subsequent patch will introduce code for building XPIs from
these templates.

Unfortunately, we didn't go 100% and make manifest.json a template. This
change would require significant refactorings to how build.py discovers
experiments. (It would need to support dynamically generated
manifest.json files.)

Comment 30

[Gregory Szorc [:gps]]

Attachment: Shared code for producing a translation experiment from templates

Comment 31

[Gregory Szorc [:gps]]

Attachment: Vietnamese translation experiment

The API key needs work. I'm talking with jakem about establishing a
file with secrets only available to the server/build process. I figure
"foo" and "bar" can be stand-ins for the moment.

Comment 32

[Gregory Szorc [:gps]]

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/a60d3ccb271c
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/875990150484
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/9475cb4d1092

Comment 33

[Gregory Szorc [:gps]]

Attachment: Allow both an XPI and a build script to exist

Comment 34

[Benjamin Smedberg]

Comment on attachment 8456356 [details] [diff] [review]
Shared code for producing a translation experiment from templates

I think it's a little strange to have experiment-specific in the telexserver/ directory. Can we either move this into a translation-specific directory, or make this generic code not specific to the translation experiment but valid for any experiment?

If it's going to be generic, perhaps build_translation_xpi should be more generic, such as

def build_translation_xpi(template_dir, dest_xpi, templateparams)

or **templateparams

Comment 35

[Benjamin Smedberg]

Comment on attachment 8456357 [details] [diff] [review]
Vietnamese translation experiment

rs=me for this once the templating patch is done

Comment 36

[Gregory Szorc [:gps]]

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/862e01e5501f

Comment 37

[Gregory Szorc [:gps]]

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/69d5c09f44c7

Comment 38

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] Away 19-July through 3-Aug from comment #34)
> Comment on attachment 8456356 [details] [diff] [review]
> Shared code for producing a translation experiment from templates
> 
> I think it's a little strange to have experiment-specific in the
> telexserver/ directory. Can we either move this into a translation-specific
> directory, or make this generic code not specific to the translation
> experiment but valid for any experiment?
> 
> If it's going to be generic, perhaps build_translation_xpi should be more
> generic, such as
> 
> def build_translation_xpi(template_dir, dest_xpi, templateparams)
> 
> or **templateparams

I'm tempted to call YAGNI until proved otherwise. We can certainly refactor later.

The reason the code exists in telexserver is because it's a reusable module. If this were other languages, I'd consider a #include equivalent. But Python *really* frowns on this pattern. I would be inviting later maintainers to curse my name. I'd prefer to save face.

Stay tuned for a new patch that hopefully addresses things.

Comment 39

[Gregory Szorc [:gps]]

Attachment: Framework for obtaining secret values

Some experiments contain secret values such as API keys. We don't want
these checked into source control. However, we don't want to make
developers' lives harder either.

This patch introduces a "secrets" file. It is simply a Python file whose
executed content gets converted into a dict. It is similar to how Django
config files and moz.build files work.

There exists an in-tree secrets file with dummy values or values we
don't care about leaking to the outside world. In production, the actual
secrets file is installed via some other mechanism and the official
build process sets an environment variable with the location of this
file.

While we'll eventually want the server to provide a secrets file, we can
avoid this extra work for the moment by checking in an .xpi file
produced with the secrets. When the server is updated, we can remove the
.xpi and use the new build mechanism.

Comment 40

[Gregory Szorc [:gps]]

Attachment: Vietnamese translation experiment

Comment 41

[Gregory Szorc [:gps]]

Attachment: Polish translation experiment

Comment 42

[Gregory Szorc [:gps]]

Attachment: Turkish translation experiment

Comment 43

[Gregory Szorc [:gps]]

I just remembered I never updated the placeholder time values in the experiment manifests. What is the run-time of this experiment? The initial comment gives a range. I need hard dates.

Comment 44

[:Felipe Gomes (needinfo for replies!)]

note: I pushed the updated template for bootstrap.js in bug 1038493.

Comment 45

[Benjamin Smedberg]

Comment on attachment 8456492 [details] [diff] [review]
Framework for obtaining secret values

This feels like overkill to me, but ok.

Comment 46

[Benjamin Smedberg]

The experiment should start immediately (whenever the 32beta upgrade happens next week), although I think that maxversion is probably incorrect: I believe we want the experiment to continue into 34 beta at least. Either Chad or Felipe needs to answer the question about the end date and total runtime.

Note: do not use maxStartTime because it's still broken on the client.

Comment 47

[Benjamin Smedberg]

Comment on attachment 8456493 [details] [diff] [review]
Vietnamese translation experiment

r=me except for figuring out the final dates/runtime. I'm pretty sure you'll need to update endTime/maxActiveSeconds/maxVersion for all of these.

Comment 48

[:Felipe Gomes (needinfo for replies!)]

Start time: Jul 16, 2014   =>  1405479600
End time:   Nov 25, 2014   =>  1416880800
maxActiveSeconds: 126 days =>  10886400
minVersion: 32.0 (no need for "a" suffix for beta)

I think maxVersion is unnecessary, but if we want to include one: 34.0

Start time will actually be July 22 when the beta channel becomes 32, but by setting it to a time in the past it makes this a bit easier to QA with no downsides..

Comment 49

[Gregory Szorc [:gps]]

Attachment: Increase max experiment duration

Translation experiments need to run for 126 days. We increase the arbitrary
42 day max to another arbitrary value, 180 days.

Comment 50

[Gregory Szorc [:gps]]

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/71f6b94e78bf
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/046f3757419c
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/23cffe0472e6
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/facc76e924c6
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/24823cac9a1b
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/3df13e883c57
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/ebe5b46e05e2

Comment 51

[Gregory Szorc [:gps]]

Everything is checked in and this should be ready for QA and final deployment.

Comment 52

[:Felipe Gomes (needinfo for replies!)]

QA and final deploy will happen on separate bugs, so marking this one as fixed.

Comment 53

[Marco Mucci [:MarcoM]]

(In reply to :Felipe Gomes (away Jul 23 - Aug 13) from comment #52)
> QA and final deploy will happen on separate bugs, so marking this one as
> fixed.

Hi Felipe, based on your comment should this bug be marked as [qa-]?

Comment 54

[:Felipe Gomes (needinfo for replies!)]

Yeah, let's call this QA- and leave that to bug 1041598.