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

[AERIE-1844] Use constraints language as subset of scheduling language #179

Conversation

mattdailis
Copy link
Collaborator

  • Tickets addressed: AERIE-1844
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

This PR adds a gradle action to copy files matching constraints-*.ts out of the constraints-dsl-compiler and into the scheduling-dsl-compiler.

It then replaces the scheduling language's state-based windows expressions with the constraints windows expressions.

Activity expressions are treated separately, because they are intended to have a slightly different semantics in the scheduler, since they are not well represented by an Expression<Windows>. We can revisit these in a future PR.

Some amount of rebasing/coordination will be required between this PR and the constraints type checking PR.

Verification

Existing scheduler tests were updated (since their assertions turned out to be incorrect). Then, the tests were updated again to match the new syntax. They now pass.

Documentation

This section in the wiki will need to be updated: https://github.com/NASA-AMMOS/aerie/wiki/Scheduling-Guide#coexistence-goal-coming-soon

Future work

  • Activity Expressions should not use a TimeRangeExpression, since we do not want to coalesce overlapping activities of the same type
  • Type checking/code generation on the constraints side will need to be threaded into the scheduling side as well

@mattdailis mattdailis changed the title [AERIE-1844] Use constraints langauge as subset of scheduling language [AERIE-1844] Use constraints language as subset of scheduling language May 19, 2022
@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch from f84daf4 to 94712f3 Compare May 19, 2022 18:41
@dyst5422
Copy link
Contributor

Interested in the choice with activity expression returning raw ast. It seems a different approach than our other eDSL operators which hold the ast nodes and provide manipulation on them. I get that there aren't currently operators for the activity expressions, but I think maintaining the same patterns is good for maintainability. Also interested in if it makes sense to extend the windows api with a during activity method rather than introduce an activity expression.

@mattdailis
Copy link
Collaborator Author

Interested in the choice with activity expression returning raw ast

I'll go ahead and make the change to make it consistent! This was the result of me not thinking, rather than a deliberate decision.

@mattdailis
Copy link
Collaborator Author

Also interested in if it makes sense to extend the windows api with a during activity method rather than introduce an activity expression.

The example that came to mind (that I'm not sure how to resolve) is if you imagine a plan with two activities of the same type, that overlap.

gantt

Type A   :         des3, 2014-01-06, 2d
Type A   :         des4, 2014-01-07, 2d

If we write a Coexistence goal For each A, place a B, how many Bs should be placed?

Currently the scheduler will place one, at the end of the second activity. I think this is probably not what a user would want..? We somehow need to retain the fact that the two activities are separate, and an Expression<Windows> will lose that information.

The Constraints language has a ForEachActivity, which is intended to handle this case, but I'm not sure how to finagle it into the scheduling language, so I decided to punt on that for now.

@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch from 94712f3 to 0d67f41 Compare May 19, 2022 19:26
@mattdailis
Copy link
Collaborator Author

mattdailis commented May 19, 2022

Interested in the choice with activity expression returning raw ast

I'll go ahead and make the change to make it consistent! This was the result of me not thinking, rather than a deliberate decision.

Done - here's the diff

EDIT: Not quite, working on fixing it now

EDIT 2: Updated diff

@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch 2 times, most recently from c106e69 to 99a95db Compare May 19, 2022 19:41
@pcrosemurgy
Copy link
Contributor

This PR adds a gradle action to copy files matching constraints-*.ts out of the constraints-dsl-compiler and into the scheduling-dsl-compiler.

This sounds like we're trying to emulate the behaviour of what I'd consider to just be a common dependency between the 2 subprojects?

@dyst5422
Copy link
Contributor

I'm not sure if I have an aversion to the copy vs common import strategy here.... Initially, I was not a fan as we have data duplication which comes with synchronization risks, but it's SO much more simple than making a common package.

I guess I'd like to see Gradle do a better job of cache invalidation to calm my fears here. (I've ran into old build info causing issues)

@pcrosemurgy
Copy link
Contributor

Is making a common package very involved? (genuine question, I'm not too familiar with Node <-> Gradle dependency wrangling)

@dyst5422
Copy link
Contributor

dyst5422 commented May 25, 2022

Well we can't guarantee a runtime file path between our subprojects as they aren't all in the same docker image, so we'd have to put it somewhere it can be pulled at installation time - typically a package repository like npm or a gitrepo, but that greatly complicates development workflows as it would require some form of publishing to pull them in - which is more likely to have them referencing different versions or just not using the latest version of the shared package.

There are other build processes we could use, but they are essentially the copy. In short, because there is no compile for Javascript, we're stuck with copying.

@pcrosemurgy
Copy link
Contributor

pcrosemurgy commented May 25, 2022

a package repository like npm or a gitrepo, but that greatly complicates development workflows as it would require some form of publishing to pull them in

This sounds pretty standard though. I believe we've done this with other TS Aerie packages?

more likely to have them referencing different versions or just not using the latest version of the shared package.

Isn't this what package-lock.json and package.json accomplish in tandem?

Edit I see what you mean about the versioning. But that's kind of a fundamental complication with versioning on any kind of project with deps. in a way.

@pcrosemurgy
Copy link
Contributor

Well we can't guarantee a runtime file path between our subprojects as they aren't all in the same docker image, so we'd have to put it somewhere it can be pulled at installation time

Couldn't each docker image have the common dependency in its runtime path? There'd be some duplication since both images would have the same common dep. but at least this wouldn't involve a manual build system copy to define a dependency.

@dyst5422
Copy link
Contributor

dyst5422 commented May 25, 2022

Couldn't each docker image have the common dependency in its runtime path? There'd be some duplication since both images would have the same common dep. but at least this wouldn't involve a manual build system copy to define a dependency.

Are you suggesting that the Docker images have a shared layer? Or that we copy it into the Docker images at build time?

Neither I think is sufficient for dev-time type checking.

@pcrosemurgy
Copy link
Contributor

Ohh true! Yeah, never mind the Docker idea.

I'd still be curious to see/know how difficult publishing a common dep. package really is. In my mind this should be a standard thing to do.

And thanks for your patience w/ helping me understand all this a bit more

Copy link
Contributor

@adrienmaillard adrienmaillard left a comment

Choose a reason for hiding this comment

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

Great work. I have tested a few random things in the UI and all is good. I only have one question below.

@dyst5422
Copy link
Contributor

Ohh true! Yeah, never mind the Docker idea.

I'd still be curious to see/know how difficult publishing a common dep. package really is. In my mind this should be a standard thing to do.

And thanks for your patience w/ helping me understand all this a bit more

What we could do is a local file package.json dependency and then copy that file to an identical path in the docker image. I'm not sure this is particularly better than just doing the file copy as Matt has here. It's just trying to fit the solution into a different box.

@pcrosemurgy
Copy link
Contributor

@dyst5422 I'll let you/Matt make the call here but that sounds preferable to relying on Gradle for this. I've been bitten by Gradle copy tasks thinking they're validated/invalidated when they're not, so something as simple as a copy-on-build can turn into a bit of a mess. Our SQL file copying ticket is a good example of this simple maneuver taking a while to nail down correctly.

@pcrosemurgy
Copy link
Contributor

And I also generally prefer to see the dependency being coded as a true dependency within the Node system instead of an ad-hoc copy.

@dyst5422
Copy link
Contributor

@dyst5422 I'll let you/Matt make the call here but that sounds preferable to relying on Gradle for this. I've been bitten by Gradle copy tasks thinking they're validated/invalidated when they're not, so something as simple as a copy-on-build can turn into a bit of a mess. Our SQL file copying ticket is a good example of this simple maneuver taking a while to nail down correctly.

100% my being on board with the copy solution means ensuring that we have it properly invalidating and tied into all appropriate Gradle actions.

The only difference between the two solutions is whether a relative path or a named module is used in package imports (ie import {} from './constraints-edsl.js'; vs import {} from 'constaints-edsl';) and whether it is Gradle or Docker orchestrating the copy.

In the Docker solution space, we additionally need to put it together it as a package (package.json) and build it into .js sources and .d.ts type definitions.

@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch 3 times, most recently from 1a3ce58 to 7794e9b Compare June 2, 2022 19:57
@mattdailis
Copy link
Collaborator Author

Latest force push is a rebase onto develop. This rebase was less trivial than most, since I ended up moving the constraints TypescriptCodeGenerationService into the constraints package. I accomplished this with these steps (each in a separate commit, for the sake of clarity):

  • Extract interface for Constraints codegen
  • Wrap Constraints codegen service in an adapter
  • Make generation method static
  • Decouple Constraints codegen service from merlin-server
  • Move Constraints codegen service into constraints package

I've also added changed the /fruit resource in banananation to be a linear resource, and added a scheduling goal test that uses it. This prompted another update to the valueSchemaIsNumeric method in the constraints codegen to allow linear resources to be used as Real profiles.

@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch 2 times, most recently from e63157b to 5902a15 Compare June 6, 2022 17:41
@mattdailis mattdailis force-pushed the feature/AERIE-1844--coexistence-goal-use-constraints-language branch from 5902a15 to 24a4d35 Compare June 7, 2022 16:15
@mattdailis mattdailis merged commit 802965a into develop Jun 7, 2022
@mattdailis mattdailis deleted the feature/AERIE-1844--coexistence-goal-use-constraints-language branch June 7, 2022 16:24
@mattdailis mattdailis mentioned this pull request Oct 28, 2022
3 tasks
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

4 participants