JacquesCarette / Drasil

Generate all the things (focusing on research software)
https://jacquescarette.github.io/Drasil
BSD 2-Clause "Simplified" License
140 stars 26 forks source link

Add `stan` and `weeder` to our dev. workflow #3710

Open balacij opened 7 months ago

balacij commented 7 months ago

Motivation: #3700

Objective: Adding more static analysis tools (here, stan and weeder) to our development workflow to help us automatically catch common mistakes.

stan and weeder require HIE files post-compilation of a project before they can be used.

Approximate steps:

  1. Tell GHC to print out the HIE information that stan and weeder need. - #3708
  2. Install stan locally.
  3. Address (or suppress) issues reported by stan.
  4. Add stan to our CI and pr_ready target of the Makefile.
  5. Add stan-related information everywhere in our New Workspace guide and the wiki, as needed.
  6. Repeat steps 2-5 for weeder.

Notes:

One more time, with extra information:

  1. Tell GHC to print out the HIE information that stan and weeder need. - #3708
  2. Install stan locally. Careful! stan must be compiled with the same GHC version we intend to use for compiling Drasil. As of #3707, we're going to use GHC 9.2.8, but I could not get stan installed locally using GHC 9.2.X, so I had to update our project further to the GHC 9.4.X series by switching our targeted stack release to lts-21.25 (you can use the script found in #3707 to quickly update this too). Afterwards, you will need to add some dependencies to our main stack.yaml file in the code folder and run stack install stan. That's what worked for me, but YMMV.
  3. Address (or suppress) issues reported by stan. After installing stan, you should run it on all of our drasil-* projects and the example projects. To get you started, you can run find . -type d -name 'drasil-*' -exec sh -c 'cd {} && stan > stan.log' \; to run stan on all drasil- libraries, and then run cat drasil-X/stan.log to read the console output for each respective library. It would be helpful to first go through each category of error and fix the reports by category rather than report. Two of our notable reports as of Apr. 2nd, 2024:
    1. Lazy data causing memory leaks. stan suggests adding the StrictData language extension to make all of our data types strict, but I found that Drasil runs into an infinite loop! This will require some debugging.
    2. Infix operators lacking fixity. We define many infix operators, many lacking a related fixity (i.e., how do we know which of + and bind tighter in the expression `a+bc`?). We will need to go through each area that related infix operators are used and come up with a binding order (fixity).
  4. Add stan to our CI and pr_ready target of the Makefile. Once stan reports no issues, we can integrate stan in our CI. You will need to add stan execution on each of the drasil-related Haskell projects in the repo as a step in our Build CI (it won't be possible in our Lint CI because it requires build-related information). This should be done by creating a new Makefile target that does all the stan-related execution, and running the new target in an extra CI step. This step should also be added as a dependency of our pr_ready Makefile target.
  5. Add stan-related information everywhere in our New Workspace guide and the wiki, as needed.
  6. Repeat steps 2-5 for weeder.
smiths commented 7 months ago

I wondered if this was correctly classified as a newcomers issue, until I read the detailed description. Sounds good. :smile:

balacij commented 6 months ago

To add to this ticket, a potential future work: investigate calligraphy (relies on the HIE information, like the earlier mentioned tools) as a replacement for our existing module+data dependency graphing tools. Module+data dependency graphs is probably a good idea, but calligraphy is capable of that (and arguable a bit prettier at that) and a bit more it seems (function dependencies, at least).

EDIT: Why is it useful? We can get a deeper understanding of our dependency graph, which is particularly interesting for learning about what all of our 'chunk transformers'+/printers rely on. In other words, we can learn about how we actually go about transforming the SRS-focused knowledge into code, very easily.

Aside: Right now, we do this in Haskell, but this is perhaps something we can do internally later on too.

NoahCardoso commented 4 months ago

stan progress update. Steps 1 and 2 from Jason's list have been completed. The problem is just fixing stan suggestions. The two most prevalent are:

JacquesCarette commented 4 months ago

For further progress on stan, maybe best to turn off those two suggestions for now?

Certainly we shouldn't be relying on lazyness in a fundamental way, so if glassbr causes an infinite loop, we're doing something very strange! But that might be very hard to find and fix.

Re: nub. Fixing these properly does need a case-by-case design decision. Again, not a fast process. We should fix it, but this might end up spawning a dozen issues (one for each structure that currently uses a list and nub is applied to it).

balacij commented 4 months ago

EDIT: As of #3801, you won't need to hack the Makefile, you can just use make X_debug, where X is the name of any case study.

Regarding enabling StrictData, it is best to work as follows (yes, this is hacky):

  1. Go into the Makefile and change the line debug: test to debug: projectile_diff (yes, our debug target needs to be improved)
  2. Go into drasil-lang, edit its respective package.yaml by adding a default-extensions list containing StrictData:
    default-extensions:
    - StrictData
  3. Run make debug (though if you have more cores to spare, GHCTHREADS=4 make debug, scaling up as you can), and ignore the myriad of THUNK_1_0 errors.

    I'm not sure why they happen, but according a blog post and a stackoverflow answer, they indicate that a thunk contains a 1 [problematic] free variable that is forcibly evaluated by the runtime at a time considered 'too late' by the runtime. This might have to do with strictness issues, but I don't fully understand what is going on here yet.

  4. <<loop>>

The only issue involves the following definition: https://github.com/JacquesCarette/Drasil/blob/3d07aae97f7174d98191ecb4723873d326e05737/code/drasil-data/lib/Data/Drasil/Concepts/Physics.hs#L65-L66

Ignoring the actual semantic contents of the chunk definition, the issue here is the self-referencing! So, let's break the definition for now:

chgInVelocity = dccWDS "chgInVelocity" (cn "change in velocity")
  (S "FIXME")
  1. Run make debug again, and projectile now works!
  2. Undo the change in step 1, and run make debug again. Now, GlassBR will break at standOffDist, and we have a more complicated trace:
*** Exception (reporting due to +RTS -xc): (THUNK_STATIC), stack trace: 
  Language.Drasil.Chunk.Concept.Core.uid,
  called from Language.Drasil.Chunk.NamedIdea.nw,
  called from Language.Drasil.Chunk.Concept.cw,
  called from Language.Drasil.Chunk.Unital.uc,
  called from Drasil.GlassBR.Unitals.standOffDist,
  called from Drasil.GlassBR.Unitals.CAF:standOffDist_q1
  --> evaluated by: Language.Drasil.Chunk.NamedIdea.nw,
  called from Language.Drasil.Chunk.Concept.cw,
  called from Language.Drasil.Chunk.DefinedQuantity.dqdWr,
  called from Language.Drasil.Chunk.Constrained.constrained',
  called from Drasil.GlassBR.Unitals.standOffDist,
  called from Drasil.GlassBR.Unitals.CAF:standOffDist_q
  --> evaluated by: Language.Drasil.Chunk.NamedIdea.nw,
  called from Language.Drasil.Chunk.Concept.cw,
  called from Language.Drasil.Chunk.DefinedQuantity.dqdWr,
  called from Language.Drasil.Chunk.UncertainQuantity.uq,
  called from Drasil.GlassBR.Unitals.standOffDist,
  called from Drasil.GlassBR.Unitals.CAF:standOffDist1
  --> evaluated by: Drasil.GlassBR.Unitals.standOffDist,
  called from Drasil.GlassBR.Unitals.CAF:standOffDist
*** Exception (reporting due to +RTS -xc): (THUNK_STATIC), stack trace: 
  Language.Drasil.Chunk.Concept.Core.uid,
  called from Language.Drasil.Chunk.NamedIdea.nw,
  called from Language.Drasil.Chunk.Concept.cw,
  called from Language.Drasil.Chunk.Unital.uc,
  called from Drasil.GlassBR.Unitals.standOffDist,
  called from Drasil.GlassBR.Unitals.CAF:standOffDist_q1
glassbr: <<loop>>
make: *** [Makefile:296: glassbr_gen] Error 1

This is interesting. See how the first part and the last part are identical? It means that the issue lies in one of those lines. With uc being "top-most", we can look at how it is used in the definition of standOffDist: https://github.com/JacquesCarette/Drasil/blob/3d07aae97f7174d98191ecb4723873d326e05737/code/drasil-example/glassbr/lib/Drasil/GlassBR/Unitals.hs#L103-L105

Thankfully, there is nothing too crazy there, let's look at the only non-trivial thing there (sD): https://github.com/JacquesCarette/Drasil/blob/3d07aae97f7174d98191ecb4723873d326e05737/code/drasil-example/glassbr/lib/Drasil/GlassBR/Unitals.hs#L371-L373

Aha! Going into the definition of sdVectorSent, we realize that sdVectorSent is a sentence that contains a reference to standOffDist. Hence, sD is based on standOffDist! We have mutually recursive data definitions. We simply cannot do that with strict data!

From here, the process to fixing all the issues is as follows:

  1. Enable StrictData in any package of your choice package, preferring something at the 'top' of the dependency chain ('top' meaning the ones most depended on)
  2. Run make debug, ignore the bountiful THUNK_1_0 errors (until we really understand what is going on there), analyze and fix root causes of all <<loop>>s that occur, perhaps fiddling with the debug target definition as you go along so that you don't need to always re-test earlier fixed examples (debug target runs all the examples consecutively -- which is unhelpful if our 7th-to-run example has issues and we are using trial-and-error-style debugging)
  3. Repeat steps 1-2 for all packages and all examples.
balacij commented 4 months ago

Also, there is enough output messages through the terminal that your console's rendering engine will be the throttle on being able to run the programs, so I would advise against using VS Code's built-in terminal emulator. It is considerably slower compared to a standard terminal and also locks up while stuff is printing to its console. I suppose it's the overhead in the rendering engine and immediate-flushing of data that is causing VS Code to be comparatively slower.

NoahCardoso commented 2 months ago

Currently, both stan and weeder have been added to the makefile and the 'Infix operators lacking fixity' stan issue mentioned in the opening comment has been fixed. I think adding strict data to drasil requires a lot of redesigns and might be bigger than just adding stan

NoahCardoso commented 2 months ago

I am currently checking out all of the unused code found by weeder (if you run make weeder it will show you any unused code). Some of it such as papers in citations properly need to be kept, even if they are not used in Drasil. However, I am sure some of it should be removed.

JacquesCarette commented 2 months ago

What is the state of this issue in general? Are stan and weeder part of our workflow, or still manual? (Not a complaint, looks like a lot of progress has been made on this.)

NoahCardoso commented 2 months ago

It is still manual. Weeder (weeder reports what code is not used) will need a further look as just because we don't use a piece of code doesn't mean it should be removed. The only thing for stan is that it wants us to make everything strict data which may take a while to do.