Problem with combining descriptors in 0.6.1

[aquamatthias]

The mesosphere/marathon project tries to migrate from accord 0.5 to 0.6.1.

During this we needed to change quite a bit of code: see https://phabricator.mesosphere.com/D677

The project compiles, but we see a lot of illegal argument exceptions in our test code. All of them have the same error description: Cannot combine description coming from this code

Anything that we can do to work around this problem? BTW: would be great to see such problems during compile time, not runtime.

Most of the problems arise from collection handling, which looks like this:

  implicit def every[T](implicit validator: Validator[T]): Validator[Iterable[T]] = {
    new Validator[Iterable[T]] {
      override def apply(seq: Iterable[T]): Result = {
        seq.zipWithIndex.foldLeft[Result](Success) {
          case (result, (item, index)) => result.and(validator(item).applyDescription(Indexed(index.toLong)))
        }
      }
    }
  }

(We started to use every instead of each - for handling the path (not needed in 0.6 any more, but I don't want to change it.)) Anything you can suggest here?

I saw #89 which is marked as fixed. Probably 0.7 already has a fix for this?

[aquamatthias]

Here is the list of failing combinations at the moment:

java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Explicit(value)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Generic(options.get(optionName))' with 'Generic("dvdi/overwritefs")'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(ArrayBuffer(Indexed(0,AccessChain(WrappedArray(Generic(readinessChecks)))), Generic(name)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(ArrayBuffer(Indexed(0,AccessChain(WrappedArray(Generic(readinessChecks)))), Generic(portName)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(WrappedArray(Generic(readinessChecks), Generic(size)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit()' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit(foo)' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Generic(options.get(optionName))' with 'Generic("dvdi/iops")'
java.lang.IllegalArgumentException: Cannot combine description 'Generic(options.get(optionName))' with 'Generic("dvdi/overwritefs")'
java.lang.IllegalArgumentException: Cannot combine description 'Indexed(0,AccessChain(WrappedArray(Generic(externalVolumes))))' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Explicit(value)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Generic(options.get(optionName))'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(1,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Explicit(value)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(ArrayBuffer(Indexed(0,AccessChain(WrappedArray(Generic(readinessChecks)))), Generic(name)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(ArrayBuffer(Indexed(0,AccessChain(WrappedArray(Generic(readinessChecks)))), Generic(portName)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'AccessChain(WrappedArray(Generic(readinessChecks), Generic(size)))' with 'SelfReference'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit()' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Explicit(foo)' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'Generic(options.get(optionName))' with 'Generic("dvdi/iops")'
java.lang.IllegalArgumentException: Cannot combine description 'Indexed(0,AccessChain(WrappedArray(Generic(externalVolumes))))' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Generic(options.get(optionName))'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(0,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(1,Empty)'
java.lang.IllegalArgumentException: Cannot combine description 'SelfReference' with 'Indexed(0,Empty)'

[holograph]

Hi @aquamatthias and thanks for the report. This shouldn't be related to #89 at all (different problems); this probably has to do with either deficiencies of the description model and/or incorrect usage. I'm dealing with a production issue right now, but I'll try to give it a serious look over the weekend.

[aquamatthias]

Great. I pushed my branch here: https://github.com/mesosphere/marathon/tree/mv/wix_06 if you want to see this in action.

sbt> test

Will throw the exceptions that I have pasted in my previous comment. Feedback highly appreciated!

[holograph]

Hi @aquamatthias, I'm pretty sure this is the result of a change from 0.5->0.6 on commit 0e606cfdc6949c77584efbc5142a90be614ffdd8, which basically reverses the order of combining indexed descriptions (for very good reasons, but then I never expected anyone to use this API quite so early).

I'm pretty confident you can simply reverse the application (Indexed(index.toLong).applyDescription...), but I couldn't get your branch to compile due to the following error:

[error] (marathon/aspectj:ajc) org.aspectj.bridge.AbortException: abort: BCException ABORT - Whilst processing type 'Lmesosphere/marathon/core/task/update/impl/steps/NotifyRateLimiterStepImpl$$anonfun$notifyRateLimiter$2$$anonfun$apply$1;' - cannot cast the outer type to a reference type.  Signature=Lmesosphere/marathon/core/task/update/impl/steps/NotifyRateLimiterStepImpl$$anonfun$notifyRateLimiter$2; toString()=mesosphere.marathon.core.task.update.impl.steps.NotifyRateLimiterStepImpl$$anonfun$notifyRateLimiter$2 class=NotifyRateLimiterStepImpl$$anonfun$notifyRateLimiter$2
[error] when processing type mungers
[error] when weaving
[error] when batch building BuildConfig[null] #Files=0 AopXmls=#0

[aquamatthias]

Hey @holograph, I updated the Marathon branch and made sure it compiles correctly (clean compile is successful). I started to write my own Descriptor combinations. This only works in situations where there is control over it (like every). I still run into this problem with standard validators.

[holograph]

@aquamatthias I'll have another look and let you know.

[holograph]

@aquamatthias Good/bad news:

1. You're reimplementing the each operator as the every function, which mixes in descriptions in the opposite direction from the DSL-generated version. This is why you're getting so many failures. You can somewhat work around this by reimplementing every with this ugly hack:

  // This should possibly be added to the Accord API
  private def reverseCombine(v: Violation, d: Description) = v match {
    case rv: RuleViolation => RuleViolation(rv.value, rv.constraint, Descriptions.combine(d, rv.description))
    case rv: GroupViolation => GroupViolation(rv.value, rv.constraint, rv.children, Descriptions.combine(d, rv.description))
  }

  implicit def every[T](implicit validator: Validator[T]): Validator[Iterable[T]] = {
    new Validator[Iterable[T]] {
      override def apply(seq: Iterable[T]): Result = {
        seq.zipWithIndex.foldLeft[Result](Success) {
          case (result, (item, index)) =>
            validator(item) match {
              case Success => result
              case f @ Failure(violations) => result and Failure(violations.map { v => reverseCombine(v, Indexed(index.toLong)) })
            }
        }
      }
    }
  }

2. This makes apparent a deficiency in Accord's description model (which is one that may have necessitated your version of Descriptions.combine); I've dug into this and managed to create a small reproduction case, which you can see in the issue109 branch at cbc74afaaef1b4868d6187d3c0b5a56b3097e414. With luck this means a 0.6.2 point release (+ possibly a PR to Marathon) will plausibly be out within the week, track here for progress updates.

[holograph]

@aquamatthias Actually I take back my comment, you were basically doing exactly the right thing except it needed a bit more scaffolding on Accord's internals. I'm running your branch (without your custom combine) against a build of the Accord issue109 branch (check out and run sbt publishLocal) and it seems the issue you described is now resolved. This exposed other deficiencies though:

marathon(mv/wix_06)> testOnly mesosphere.marathon.api.v2.ModelValidationTest
...
*** 1 TEST FAILED ***
ModelValidationTest:
ModelValidation should
- Validators should not produce 'value' string at the end of description. *** FAILED *** (35 milliseconds)
  java.lang.IllegalArgumentException: Cannot combine description 'Indexed(1,Explicit(apps))' with 'Indexed(0,Empty)'
  at com.wix.accord.Descriptions$.com$wix$accord$Descriptions$$failed(Descriptions.scala:84)
  at com.wix.accord.Descriptions$$anonfun$1.apply(Descriptions.scala:111)
  at com.wix.accord.Descriptions$$anonfun$1.apply(Descriptions.scala:91)
  at mesosphere.marathon.api.v2.Validation$class.mesosphere$marathon$api$v2$Validation$$mapViolation(Validation.scala:72)
  at mesosphere.marathon.api.v2.Validation$$anon$5$$anonfun$apply$3$$anonfun$apply$4.apply(Validation.scala:63)
  at mesosphere.marathon.api.v2.Validation$$anon$5$$anonfun$apply$3$$anonfun$apply$4.apply(Validation.scala:63)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)

It's an interesting challenge to juggle Accord's "built in" description requirements with your custom code, but I'm happy to have such a comprehensive test case, so thank you :-) This will probably require a few more hours of work to sort out properly, but I'm hopeful that I'll be done with it in a day or two.

[aquamatthias]

@holograph that sounds fantastic! Thanks so much for taking the time! It is absolutely possible, that there are other issues in the Marathon code base that yield failed tests. The new Description types now make wrong assumptions in the tests visible.

So do you think I can get rid of the custom combine function? Can I now use the every function as written in my first comment or do I need custom Descriptor handling?

I will continue the work on the Marathon side hopefully early next week and would use your branch until there is a new version.

Thanks again for the effort!

[holograph]

@aquamatthias The custom combine should no longer be necessary (I haven't worked out all the kinks yet) and every can be replaced with the DSL each operator. There's some work to be done there as each is only available within the DSL and I've noticed you're using every externally, as with isTrue(...) and every(...); the DSL is intended to make users' lives easier but that should certainly not prevent users from interacting with the lower level primitives.

I'll likely spend a couple more days completing the work on Accord itself, so that your codebase works as intended, and will try to find the time to submit a PR replacing most the Validation.someHelper usages with Accord 0.6+ built-ins (Scala conditionals support, each etc.)

[aquamatthias]

@holograph I checked out branch issue109, published locally a 0.7-SNAPSHOT version and compiled Marathon against this version. I fixed all issues on the Marathon side. 8 test cases are failing - all with the same error:

  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit()' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(value)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Explicit(foo)' with 'Indexed(0,Empty)'
  java.lang.IllegalArgumentException: Cannot combine description 'Generic(external/provider)' with 'Indexed(0,Empty)'

I updated rebased and pushed my branch: https://github.com/mesosphere/marathon/compare/mv/wix_06?expand=1

If you check out this branch, sbt test should give you exactly this result.

Some other findings:

• foo is valid which takes an implicit validator, introduces an additional SelfReference. So it is not the same same as foo is mySpecificValidator . I introduced a cleanPath method to get rid of this: https://github.com/mesosphere/marathon/compare/mv/wix_06#diff-4689b225edc1443ddfeff900869604fcR257
• The same problem arises on sequences, where the index is put on SelfReference. I introduced this rule: https://github.com/mesosphere/marathon/compare/mv/wix_06#diff-4689b225edc1443ddfeff900869604fcR259
• and also AccessChain has to be cleaned up: https://github.com/mesosphere/marathon/compare/mv/wix_06#diff-4689b225edc1443ddfeff900869604fcR255
• Is there a way to reference items in a map? We would need some kind of Indexed but not a position but a key.

Any recommendations here?

It would be great, if you could take a look into the remaining combination problems. I started a silly approach by adding:

case ( lhs: Explicit, Indexed(index, Empty ) ) => Indexed(index, lhs)
case ( lhs: Generic, Indexed(index, Empty ) ) => Indexed(index, lhs)

which is wrong since it adds the index at the wrong place. Example: Expected problem at path: /container/volumes(0)/external/provider Actual reported path: /container/volumes/external/provider(0)

[holograph]

@aquamatthias Sorry, I've had to drop everything and deal with a few other things, I expected to have it wrapped up last week. I'll follow up as soon as I can.

[aquamatthias]

After playing around a little more, these 2 rules solved my issues:

case (lhs: Explicit, Indexed(index, Empty)) => AccessChain(Indexed(index, SelfReference), lhs)
case (lhs: Generic, Indexed(index, Empty)) => AccessChain(Indexed(index, SelfReference), lhs)

Does this make sense?

[holograph]

Not quite, actually; Explicit overrides whatever is the previous description, and (in the case of runtime application) an open Indexed applies to the existing description, so I'd expect the following:

case (lhs: Explicit, Indexed(index, Empty)) => Indexed(index, lhs)
case (lhs: Generic, Indexed(index, Empty)) => Indexed(index, lhs)

These should be covered by the baseline rules. I think the problem stems from trying to support indexing from both directions (i.e. as part of the DSL -- meaning an open Indexed has the actual description applied to it -- and as part of the runtime validation helpers, meaning an open Indexed may be applied to an existing description). I may have to rework the types to differentiate the two, which will make everything a lot easier to work with.

[aquamatthias]

@holograph Please see my earlier comment, where I already tried your suggestion:

case ( lhs: Explicit, Indexed(index, Empty ) ) => Indexed(index, lhs)
case ( lhs: Generic, Indexed(index, Empty ) ) => Indexed(index, lhs)

This leads to wrong results. since it adds the index at the wrong place. Example (with path based syntax to show the problem): Expected problem at path: /container/volumes(0)/external/provider Actual reported path: /container/volumes/external/provider(0) Volumes is a sequence, provider is a simple string.

The root cause of this is probably the "inverse" as you pointed out: the index is on self (and not on the left hand side) and needs to be combined with a child violation.

That's why I defined the combine operation like this:

case (lhs: Explicit, Indexed(index, Empty)) => AccessChain(Indexed(index, SelfReference), lhs)
case (lhs: Generic, Indexed(index, Empty)) => AccessChain(Indexed(index, SelfReference), lhs)

Since the "parent reference" (the sequence) is not part of the combination, SelfReference is used.

[Antanukas]

Seems that I have same IllegalArgumentException, but for simpler case: https://github.com/Antanukas/accord-reproduce/blob/master/src/test/scala/reproduce/ReproduceTest.scala

[holograph]

@Antanukas Thanks for the additional use case, and I apologize for the tardiness in getting this wrapped up (this goes for @aquamatthias as well). I'll follow up as soon as I'm able.

[holograph]

Just following up that there's been significant progress on this. Digging through it basically made me realize the whole description algebra in 0.6 is insane, and there's significant progress towards a much cleaner model for 0.7 (see the prototype-0.7 branch). I'm actively developing this against Marathon, so I'll hopefully have a full-blown pull request ready along with the new API :-)

Sorry for the migration mess, this is exactly why I don't consider Accord 1.0 yet.

[holograph]

@aquamatthias Managed to make Marathon fully test against 0.7-SNAPSHOT (not yet published, I publishLocal and depend on that). You can see the changes on my fork, and hopefully it makes things simpler and cleaner for Marathon as well.

The next steps on my part:

1. Finish and cleanup 0.7
2. Adjust Marathon to make use of new features (e.g. the Path type, instead of the awkward group violation business)
3. Document, document, document
4. Release

Your feedback has been invaluable so far and would be greatly appreciated. I'd rather avoid going through another description algebra rework, and it's really great to have a concrete use case to work with!

[aquamatthias]

@holograph that looks promising. 1) I think replacing Description by Path makes a lot more sense - it really denotes the path to the violation. 1) is there still a difference between item is valid and item is mySpecificValidator? The former created an unnecessary SelfReference in the path. Ideally the 2 versions have the same result. 1) What is the suggested way of testing validations? Constructing a complete path was pretty tedious with Descriptions. See my simplification in UnitTest, which uses the rendered path (Marathon only). A simple way in accord would help. 1) I am still unsure how to reference an item by name: think of a Map, where the numeric index does not give define a proper path.

Looking forward to 0.7! //cc: @jeschkies

[holograph]

@aquamatthias, @jeschkies:

1. Yes, the move to Path really simplified a lot of things, which wasn't evident until I took a longer look at how Marathon does things (ah, the benefits of having actual users!)
2. It should, but I'll add a test to verify.
3. That is actually a good question; Accord is notably unopinionated here, with some users at Wix treating it as "infrastructure, no need to test" and others doing pervasive unit/integration testing to make sure validations behave as expected. I suspect Marathon belongs that to the latter category because validation errors are actually exposed to the client, so it obviously needs a more rigorous testing regiment. There's an old standing wishlist item of mine, #26, that would provide a much better story for testing; I'll try to follow up on this question next week (after ScalaSwarm).
4. Indexes should probably not be numeric to support maps or other associative structures, although that runs the risk of introducing a mess into descriptions (e.g. in the case of long or binary keys). I don't see an easy way around it though, and will look into making this change in 0.7 and subsequently Marathon.

Cheers and thanks for sticking with this!

[holograph]

All changes are now merged to master! @aquamatthias, is there any additional support you need (other than index-by-key, which I'll consider a separate issue for now) prior to 0.7 release?

[aquamatthias]

@holograph 👍 we will try 0.7 in Marathon. Index by key: I agree this is a separate issue. Thanks for the effort!