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

Add stan and weeder to our dev. workflow #3710

Open
balacij opened this issue Apr 3, 2024 · 10 comments
Open

Add stan and weeder to our dev. workflow #3710

balacij opened this issue Apr 3, 2024 · 10 comments
Labels
newcomers Good first issue to work on!

Comments

@balacij
Copy link
Collaborator

balacij commented Apr 3, 2024

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. - Write hie info #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:

  • This should be done incrementally! Send in many small PRs.
  • Step (4) might be less-than-fun, so I can help out with it.

One more time, with extra information:

  1. Tell GHC to print out the HIE information that stan and weeder need. - Write hie info #3708
  2. Install stan locally. Careful! stan must be compiled with the same GHC version we intend to use for compiling Drasil. As of Update stackage release: 20.20 (GHC 9.2.7) -> 20.26 (GHC 9.2.8) #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 Update stackage release: 20.20 (GHC 9.2.7) -> 20.26 (GHC 9.2.8) #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+b*c?). 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.
@balacij balacij added the newcomers Good first issue to work on! label Apr 3, 2024
@smiths
Copy link
Collaborator

smiths commented Apr 3, 2024

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

@balacij
Copy link
Collaborator Author

balacij commented Apr 26, 2024

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
Copy link
Collaborator

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:

  • Adding strict data expressions. The problem is after I add strict data to drasil-database I get an infinite loop from glassbr. I found an infinite loop used in ChunkDB which I think is the problem but changing it doesn't fix the problem.
  • replacing nub with toList . fromList. stan doesn't like our use of nub since it is slow and suggests just converting to a set and then back to a list which is faster than using nub. The problem is would it be better just to use a set rather than a list? When should we use a set over a list or list over a set?

@JacquesCarette
Copy link
Owner

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).

JacquesCarette added a commit that referenced this issue Jun 6, 2024
@balacij
Copy link
Collaborator Author

balacij commented Jun 13, 2024

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
  1. 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.

  2. <<loop>>

The only issue involves the following definition:

chgInVelocity = dccWDS "chgInVelocity" (cn "change in velocity")
(S "the" +:+ phraseNP (chgInVelocity `ofA` rigidBody))

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:

standOffDist = uq (constrained' (uc sD (variable "SD") Real metre)
[ gtZeroConstr,
sfwrc $ Bounded (Inc, sy sdMin) (Inc, sy sdMax)] (exactDbl 45)) defaultUncrt

Thankfully, there is nothing too crazy there, let's look at the only non-trivial thing there (sD):

sD = cc' stdOffDist
(S "the distance from the glazing surface to the centroid of a hemispherical" +:+
S "high explosive charge. It is represented by the coordinates" +:+ sParen sdVectorSent)

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
Copy link
Collaborator Author

balacij commented Jun 13, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator

NoahCardoso commented Aug 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
newcomers Good first issue to work on!
Projects
None yet
Development

No branches or pull requests

4 participants