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

WX-1461 Remove womtool upgrade command #7382

Merged
merged 19 commits into from
Mar 12, 2024
Merged

WX-1461 Remove womtool upgrade command #7382

merged 19 commits into from
Mar 12, 2024

Conversation

aednichols
Copy link
Collaborator

Also removes large chunks of the WDL writer that were only used for very specific error messages.

@@ -63,7 +58,7 @@ object LinkedGraphMaker {
// we want to start the cycle with the edge "a -> b"
val edgeDict: Map[String, String] =
cycle.value.edges.map { case graph.EdgeT(from, to) =>
nodeName(from) -> nodeName(to)
from.value.toString -> to.value.toString
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These error messages for cyclical graphs used to look like

call a5.add5 {

complete with mismatched { because nodeName() wrote out the whole call and took the first line. Now looks like

Call "a5.add5"

so hardly any difference and this allows us to drop the entire WdlWriter[WorkflowGraphElement] and all of its subtypes.

Comment on lines +60 to 63
private def attributesToString: String = attributes.toWdlV1
// Yes, it's sad that we need to put ${} here, but otherwise we won't cache hit from draft-2 command sections
// Re-writes e.g. `echo $((5 + ~{x.a}))` commands to `echo $((5 + ${x.a}))` so they evaluate to equal
override def toString: String = "${" + s"$attributesToString${expression.expressionElement.toWdlV1}" + "}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is worth it or if I even agree with the original decision, but adjudicating that seemed out of scope for this PR. It does mean we have to retain the ability to write any expression element.

@@ -18,7 +18,7 @@ import wdl.model.draft3.graph.ExpressionValueConsumer.ops._
import wdl.model.draft3.graph.expression.WomExpressionMaker.ops._
import wdl.transforms.base.linking.expression._
import wdl.transforms.base.wdlom2wdl.WdlWriter.ops._
import wdl.transforms.base.wdlom2wdl.WdlWriterImpl._
import wdl.transforms.base.wdlom2wdl.WdlWriterImpl.{expressionElementWriter, placeholderAttributeSetWriter}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a workflow thing that made it easier for me to see what is really being used or not.

@aednichols aednichols marked this pull request as ready for review March 11, 2024 17:34
@aednichols aednichols requested a review from a team as a code owner March 11, 2024 17:34
Comment on lines -47 to -62
testFiles.foreach { file =>
it should s"write a file that re-evaluates to the same case classes for ${file.name}" in {

val model: Checked[FileElement] = (fileToAst andThen wrapAst andThen astToFileElement).run(file)
model match {
case Right(wdlModel) =>
val newModel = (stringToAst andThen wrapAst andThen astToFileElement).run(
FileStringParserInput(wdlModel.toWdlV1, file.name)
)

// Scala case class deep equality is so nice here
newModel.map(stripSourceLocations) shouldEqual model.map(stripSourceLocations)
case Left(_) => fail("Could not load original")
}
}
}
Copy link
Collaborator Author

@aednichols aednichols Mar 11, 2024

Choose a reason for hiding this comment

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

I remain proud of this code, which...

  1. parses a WDL on disk to classes
  2. writes out the classes with the WDL writer
  3. re-parses the written-out version
  4. compares the new classes with those in step (1)

Copy link
Contributor

@jgainerdewar jgainerdewar left a comment

Choose a reason for hiding this comment

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

So much red, you love to see it.

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@

## 87 Release Notes

### `upgrade` command removed from Womtool

Womtool previously supported a `womtool upgrade` command for upgrading draft-2 WDLs to 1.0. With WDL 1.1 taking over as the latest supported version, this functionality is retiring.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to hedge this a little more, like "With WDL 1.1 soon to become the latest supported version"

@aednichols aednichols merged commit 1049c4e into develop Mar 12, 2024
34 checks passed
@aednichols aednichols deleted the aen_wx_1461 branch March 12, 2024 21:01
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

3 participants