Bump planout.js major version to v4.0.0 #58

mattrheault · 2017-03-12T15:25:21Z

This PR consists of changes from the following PR's:

V4.0 Changes:

Core compatible bundle fixes:
- Core compatible namespace allocations now match core reference namespace allocations from python version of planout.
- Core compatible interpreted experiment enrollment now matches core reference interpreted experiment enrollment from python version of planout.
Separated the concerns of planout core random operations, and the planout API. The planout.js API (experiment, assignment, namespace, etc) are now composed with the random operations they are passed.
Added planoutAPIFactory.js to keep planout bundles consistent.
Made experiment & namespace names required to fix Rethink namespace / experiment name as part of assignment algorithm in JS port only #57
Fixes WeightedChoice with false-y choices.
Fixes tests on windows + running the travis tests on windows as well if this is possible.

This resolves an issue where the core-compatible Namespace segment allocation would erroneously use the non-compatible implementation instead.

This removes cases where we would require() src code in the unit tests and then try to intertwine those objects with built library code. We should only be using built library code in the unit tests. Effects of this commit: * PlanOut.Ops.Core is no longer undefined * Exposed the following additional modules on PlanOut: * PlanOut.Lib.Utils * PlanOut.Ops.Base * PlanOut.Ops.Utils * Unit tests now pass on Windows machines One trade-off here is that we now expose some previously internal modules to clients of the library. In some cases, it seems like the right thing to do. For instance, the unit tests demonstrate how to register a custom operator, but without PlanOut.Ops.Base one cannot replicate that in application code. In other cases, like PlanOut.Lib.Utils, it seems like we might be exposing too much. I think it may still be ok to do this, as anyone using this library will have better alternatives for the internal functionality (lodash, underscore), but I can understand if we want to find an alternative solution.

This resolves an issue that would prevent interpreted Experiments from working correctly in core-compatibility mode. The problem was that the Interpreter class uses the operations table defined in Ops.Utils and that table is initialized with the non-compatible operation code. This code allows the build/index_core_compatible.js module to apply the correct overrides for a core-compatible build. This also corrects two unit tests that should have caught this issue.

This centralizes the Random code differences to just Ops.Random and Ops.Utils. We make Namespace look up these operations from the same place as the Interpreter and other classes, rather than being imported directly.

For modules that export more than one item, we now use the `export {...}` syntax. The corresponding imports are updated to the `import * as <something> from ...` syntax. This makes it more clear and consistent that each module is exporting a collection of items rather than a single item with a collection of properties.

It turns out we don't need to risk exposing these yet. They were previously exposed for unit test purposes, but an alternative solution has been found - use requireActual() instead of require() to load the Lib.Utils code for usage during the unit tests.

It doesn't make sense to create a new instance of BigNumber() for each operator instance (in compat code), or to waste memory on what's essentially a constant (in both).

This keeps our index.js and index_core_compatible.js files as similar as possible while still allowing them to specialize their behavior by selecting the appropriate Random library to use.

…e setup()

Jcwilson fix core compatibility

…names Requiren name to be set from within experiment & namespace setup()

jcwilson · 2017-03-12T20:20:27Z

It occurs to me that Experiment.getDefaultParamNames() will suffer from the same problems as the removed getDefaultNamespaceName() and getDefaultExperimentName() in #57. It tries to examine the assign() function's code and does a lexical search for set( in order to extract param names. The set() function will potentially get munged during minification.

EDIT: actually, we may be safe here. It wouldn't make sense for a minification step to alter the public interface of an object. I still think that function is a kind of trap since it depends on the client writing code in a particular way so as to enable the discovery. At least it must be explicitly invoked by the client and it's not a liability in the only place the library calls it internally. Still it feels weird to still have code that tries to introspect javascript function bodies to accomplish its task.

mattrheault · 2017-03-12T23:08:38Z

@jcwilson I think we'll be safe there since things like object properties / methods shouldn't be munged in a minification process (doing so would break any runtime code that depends on specific property / method names). Though I do want to find a way to make the implementation of getDefaultParamNames() less magical - #56

jcwilson · 2017-03-12T23:11:26Z

I guess the question is whether or not we hold up the v4 release over it. I'd be ok leaving it as-is for now.

mattrheault · 2017-03-12T23:26:57Z

I think we'll be okay to move forward with the v4 release before tackling that issue. The toString() matching is gross, but I don't think it's likely to affect a lot of people. And if it does affect people, then it will only hinder autoexposure logging (enrollment & assignment would be unaffected). For context, I left a followup comment in #56 explaining how we ran into this problem.

jcwilson · 2017-03-12T23:57:40Z

Cool, sounds good.

And yeah, that example you posted in #56 was exactly why I called it a "trap" :)

rawls238 · 2017-03-13T19:28:03Z

Yes it is in fact immune to minification

rawls238 · 2017-03-15T02:15:02Z

@mattrheault is this ready for review or are you planning on adding more to this?

rawls238 · 2017-03-15T02:26:36Z

es6/ops/randomPlanoutCoreCompatible.js

-  BernoulliTrial: BernoulliTrialBuilder(PlanOutOpRandomCoreCompatible),
-  RandomInteger: RandomIntegerCoreCompatible,
-  RandomFloat: RandomFloatBuilder(PlanOutOpRandomCoreCompatible)
+var WeightedChoiceCoreCompatible = WeightedChoiceBuilder(PlanOutOpRandomCoreCompatible);


moving this comment from the other PR: should this be const or let instead? I know this project is littered with var but we may as well start moving towards more es6 style

rawls238 · 2017-03-15T03:01:24Z

copying this in here since I wanted to respond to this but it seems to have been lost since it was an inline-comment

Unfortunately I think we're wrestling a bit with the current structure of the project to force it to work the way that we want. In an ideal world, we shouldn't have to apply any "overrides" to our random operations at runtime.

The direction that I think PlanOut.js should move in is one where we totally separate the concerns of API & random operations. PlanOut.js's experiments, namespaces, interpreter, etc should be capable of being composed from the top with whatever random operations we choose. Right now we're in an awkward middle ground where some stuff is composable, but other stuff directly imports random operations.

Here's a peak into what I'm imagining:

/
/build
index.js
index_core_compatible.js
/es6
/api
...
/ops
...
I'm not married to this exact directory structure, but the important detail is that we should have a hard rule that no module in /api will ever import directly from /ops. Anything that an api module needs from /ops should be passed down from the build's entry point.

With this restructuring, I think that PlanOut.js will be in a good place to scale without having to worry about future changes causing a shift in enrollment for either the compat or non-compat builds.

Taking a step back to the problem at hand...

The fact is that things are broken, and they need to be fixed. The question I'm interested in answering right now is - do we apply a bandaid to fix core-compat, or do we prioritize a more involved project restructure?

@jcwilson do you have any thoughts on the matter? How high priority is fixing core-compat to you and your team?

@rawls238 do you think that what I'm proposing sounds like a reasonable direction to head in?

/cc @geoffdaigle

I think it's best to not overcomplicate things with core-compat.

First, I don't think that many people (if any) require compatibility and the non-compat version is substantially faster than the compat version. I'd be interested to hear about @jcwilson's use-case with respect to this.

Second, I don't believe it would be desirable to untangle random operations from everything since namespaces directly rely on random ops to do namespace enrollment (as they should) and the assignment class executes the random operators so I don't see how this would be a significant improvement. It's not clear to me how it would be possible to divorce the two - if there are any issues with randomization they necessarily will always impact enrollment based on the current implementation and it isn't clear to me how changing around directory structure or how random ops are injected into the build would make any difference (I have a feeling I'm missing something here, so if I am let me know).

I think that @jcwilson's changes are sufficient for fixing the issue at hand and present a good framework for moving forward and I don't think a project overhaul is necessary. It is also worth pointing out that these kinds of things should be incredibly infrequent since the reference implementation rarely, if ever, will change these core aspects of the project so it seems like we would be overengineering to completely refactor the project to accommodate the slim chance that we need to deal with this issue again.

jcwilson · 2017-03-15T17:04:30Z

First, I don't think that many people (if any) require compatibility and the non-compat version is substantially faster than the compat version. I'd be interested to hear about @jcwilson's use-case with respect to this.

One of the core benefits of Planout is its ability for clients and servers to have consistent hashing without having to communicate the namespace/experiment data at runtime, say via an api call or server-side rendered template. In our case, we have also have tooling built around the Python implementation that dev and analytics can use to quickly identify if a particular user is selected for an experiment. In our case, we value that consistency over the speed benefits provided by the default distribution of PlanOut.js (though we do appreciate that effort).

My understanding was that this PR accomplishes @mattrheault's goals with regards to any refactoring, so I think we're good there.

mattrheault · 2017-03-15T17:19:33Z

@rawls238

First, I don't think that many people (if any) require compatibility and the non-compat version is substantially faster than the compat version. I'd be interested to hear about @jcwilson's use-case with respect to this.

I think the core compat version is more likely to be used in a node.js environment where the performance loss can be mitigated.

Second, I don't believe it would be desirable to untangle random operations from everything since namespaces directly rely on random ops to do namespace enrollment (as they should) and the assignment class executes the random operators so I don't see how this would be a significant improvement. It's not clear to me how it would be possible to divorce the two - if there are any issues with randomization they necessarily will always impact enrollment based on the current implementation and it isn't clear to me how changing around directory structure or how random ops are injected into the build would make any difference (I have a feeling I'm missing something here, so if I am let me know).

Namespace classes directly importing random operations is fine until we run into a situation where we have multiple bundles where random operations differ. Now that this is the case, things like Namespaces & Experiments should be handed the random operations they need to do their job, rather than importing random operations directly. The restructuring to make this happen isn't too heavy handed IMO, and I don't anticipate any negative side effects. The only thing this effects is the developer experience when contributing to planout.js.

We are running into real breaking problems here because of these imports. I disagree in that what we're doing here is a major overhaul. It's more of a minor refactor around where dependencies come from, which will alter enrollment, and thus will require a major version bump.

Does this clear things up?

rawls238 · 2017-03-15T23:05:34Z

es6/assignment.js

-  constructor(experimentSalt, overrides) {
-    if (!overrides) {
-      overrides = {};
+export default function provideAssignement(Random) {


typo here, this should be provideAssignment

rawls238 · 2017-03-15T23:10:21Z

One of the core benefits of Planout is its ability for clients and servers to have consistent hashing without having to communicate the namespace/experiment data at runtime, say via an api call or server-side rendered template. In our case, we have also have tooling built around the Python implementation that dev and analytics can use to quickly identify if a particular user is selected for an experiment. In our case, we value that consistency over the speed benefits provided by the default distribution of PlanOut.js (though we do appreciate that effort).

Cool to hear you're doing this! Theoretically this is one of the benefits of using PlanOut but in practice virtually all of the devs that I've found are using it are using a single implementation of it. Glad to see you guys are using multiple implementations!

I think the core compat version is more likely to be used in a node.js environment where the performance loss can be mitigated.

This is not recommended practice. Nobody should be using core compat mode unless they need compatibility, whether on the client or server the performance differences are substantial.

Namespace classes directly importing random operations is fine until we run into a situation where we have multiple bundles where random operations differ. Now that this is the case, things like Namespaces & Experiments should be handed the random operations they need to do their job, rather than importing random operations directly. The restructuring to make this happen isn't too heavy handed IMO, and I don't anticipate any negative side effects. The only thing this effects is the developer experience when contributing to planout.js.

We are running into real breaking problems here because of these imports. I disagree in that what we're doing here is a major overhaul. It's more of a minor refactor around where dependencies come from, which will alter enrollment, and thus will require a major version bump.

I see I see, I was envisioning that you were saying something else (which in my mind made no sense) but this makes a lot of sense. I think in the short-term we should stick to these changes as I don't think they're a band-aid fix but rather a sensible restructuring. In the long-term I'd be interested to see the implementation of something like this and see if it makes things easier on the dev side than the current implementation. The only worry I have is doing something like that and breaking things for no performance or external usability reasons but I think this project has good enough test coverage where it should be OK. Up to you if you want to attempt at doing so, I'm not sure how big the gains would actually be but they seem positive to me.

Doing this after merging this in shouldn't require a major version bump since enrollment won't be affected if we do this kind of refactoring after this fix has already been applied.

rawls238

Besides the few nit comments, I think this looks good to go. Thanks for doing this @jcwilson and @mattrheault for wrapping it up.

I would also add in the CHANGELOG:

Fixes WeightedChoice with false-y choices
Fixes tests on windows + running the travis tests on windows as well if this is possible

rawls238 · 2017-03-15T23:05:36Z

es6/assignment.js

-  constructor(experimentSalt, overrides) {
-    if (!overrides) {
-      overrides = {};
+export default function provideAssignement(Random) {


typo here, this should be provideAssignment

weird. not sure how that lets anything work unless the import/export stuff doesn't care about the name here

It's because the named export is the default export, which you can import as whatever name you choose. The main advantage of naming a default export function is that it'll make stack traces a bit easier to follow when the code is compiled & minified.

jcwilson · 2017-03-16T17:10:16Z

Thanks for everyone's help on this. I'm excited to start using v4.0! 💯

rawls238 · 2017-03-17T17:20:58Z

@jcwilson would you prefer that we put the compat planout implementation as its own package on npm?

Josh Wilson and others added 20 commits March 5, 2017 18:18

Make Namespace core-compatible again

34aae16

This resolves an issue where the core-compatible Namespace segment allocation would erroneously use the non-compatible implementation instead.

Remove namespacePlanoutCoreCompatible.js specialization

acd2b60

This centralizes the Random code differences to just Ops.Random and Ops.Utils. We make Namespace look up these operations from the same place as the Interpreter and other classes, rather than being imported directly.

Apply dependency inversion principles

7d3df76

Refactor random to split out base and compat code

200f818

Remove LONG_SCALE as a property on PlanOutOpRandom

f4e1f9e

It doesn't make sense to create a new instance of BigNumber() for each operator instance (in compat code), or to waste memory on what's essentially a constant (in both).

Clean up random.js and randomBase.js

a66d84a

Rename initializeOperations() to initializeOperators()

41a1c37

Introduce planoutAPIFactory

0d193fe

This keeps our index.js and index_core_compatible.js files as similar as possible while still allowing them to specialize their behavior by selecting the appropriate Random library to use.

Bump planout.js major version to v4.0.0

78a6420

Require this.setName() to be called from within experiment & namespac…

6c2f2fe

…e setup()

Update readme to include experiment setName example

5ceffac

Merge pull request #60 from HubSpot/jcwilson_fix_core_compatibility

285ed39

Jcwilson fix core compatibility

Merge branch 'v4' into require_experiment_and_namespace_names

e3793a6

Merge pull request #59 from HubSpot/require_experiment_and_namespace_…

e864766

…names Requiren name to be set from within experiment & namespace setup()

Add changelog notes

eb591c3

Bump bower package version

bc2437d

This was referenced Mar 12, 2017

Make planout_core_compatible actually core-compatible again #54

Closed

Rethink namespace / experiment name as part of assignment algorithm in JS port only #57

Closed

jcwilson approved these changes Mar 12, 2017

View reviewed changes

rawls238 reviewed Mar 15, 2017

View reviewed changes

Var -> const

0c9ae04

rawls238 reviewed Mar 15, 2017

View reviewed changes

rawls238 approved these changes Mar 15, 2017

View reviewed changes

Matthew Rheault added 2 commits March 16, 2017 10:15

Fix typo (assignement -> assignment)

c32fb6b

Updated planout v4.0 changelog

27bb1ac

mattrheault merged commit 898c00c into master Mar 16, 2017

mattrheault deleted the v4 branch March 16, 2017 14:20

mattrheault mentioned this pull request Mar 16, 2017

planout_core_compatible namespaces are not core compatible #52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump planout.js major version to v4.0.0 #58

Bump planout.js major version to v4.0.0 #58

mattrheault commented Mar 12, 2017 •

edited

jcwilson commented Mar 12, 2017 •

edited

mattrheault commented Mar 12, 2017

jcwilson commented Mar 12, 2017

mattrheault commented Mar 12, 2017

jcwilson commented Mar 12, 2017

rawls238 commented Mar 13, 2017

rawls238 commented Mar 15, 2017

rawls238 Mar 15, 2017

mattrheault Mar 15, 2017

rawls238 commented Mar 15, 2017 •

edited

jcwilson commented Mar 15, 2017

mattrheault commented Mar 15, 2017

rawls238 Mar 15, 2017

rawls238 commented Mar 15, 2017

rawls238 left a comment

rawls238 Mar 15, 2017

jcwilson Mar 15, 2017 •

edited

mattrheault Mar 16, 2017

jcwilson commented Mar 16, 2017

rawls238 commented Mar 17, 2017

Bump planout.js major version to v4.0.0 #58

Bump planout.js major version to v4.0.0 #58

Conversation

mattrheault commented Mar 12, 2017 • edited

jcwilson commented Mar 12, 2017 • edited

mattrheault commented Mar 12, 2017

jcwilson commented Mar 12, 2017

mattrheault commented Mar 12, 2017

jcwilson commented Mar 12, 2017

rawls238 commented Mar 13, 2017

rawls238 commented Mar 15, 2017

rawls238 Mar 15, 2017

Choose a reason for hiding this comment

mattrheault Mar 15, 2017

Choose a reason for hiding this comment

rawls238 commented Mar 15, 2017 • edited

jcwilson commented Mar 15, 2017

mattrheault commented Mar 15, 2017

rawls238 Mar 15, 2017

Choose a reason for hiding this comment

rawls238 commented Mar 15, 2017

rawls238 left a comment

Choose a reason for hiding this comment

rawls238 Mar 15, 2017

Choose a reason for hiding this comment

jcwilson Mar 15, 2017 • edited

Choose a reason for hiding this comment

mattrheault Mar 16, 2017

Choose a reason for hiding this comment

jcwilson commented Mar 16, 2017

rawls238 commented Mar 17, 2017

mattrheault commented Mar 12, 2017 •

edited

jcwilson commented Mar 12, 2017 •

edited

rawls238 commented Mar 15, 2017 •

edited

jcwilson Mar 15, 2017 •

edited