Skip to content

Remove fillReqs; the only source of systemic chunk overwrites right now#4351

Closed
balacij wants to merge 2 commits intomainfrom
rmFillReqs
Closed

Remove fillReqs; the only source of systemic chunk overwrites right now#4351
balacij wants to merge 2 commits intomainfrom
rmFillReqs

Conversation

@balacij
Copy link
Collaborator

@balacij balacij commented Aug 29, 2025

Closes #4345

This PR is a "move sideways" or perhaps a "step backwards."

For reviewing, all of the Haskell changes are in d71b856 while the stable/ changes are in 0437794. For the stable/ changes, I would filter out everything but HTML files. I provide a screenshot of some diff line counts for reference here too:

Screenshot 2025-08-28 at 11 50 31 PM

fillReqs is used almost exclusively for 1 thing: automatically getting the FRs that drasil-docLang generates, and placing said FRs in the ChunkDB. fillReqs is our only systemic source of within-table chunk overwrites in the ChunkDB.

Looking at our logs, we normally only have 1 new chunk found and placed in the ChunkDB: the "input-values" functional requirement. So, in this PR, I removed fillReqs in favour of manually placing the 1 FR chunk in all of our ChunkDBs. Unfortunately, not all of our examples currently had this generic input FR requirement, so it adds a "bad" FR to GamePhysics and SSP.

Now, what this PR really makes me wonder is: should functional requirements be a chunk at all, as they are now (captured with a ConceptInstance)? Functional requirements should be somewhat conveyed somehow through the System we're trying to flesh out with #4332, #4304, and #3260 .

As an aside note: we hack many things with the input-values FR, which is why I'm okay with submitting this PR, despite knowing I'm just worsening another hack in favour of avoiding another problem. In particular, we cheat references to the input requirement with inReq EmptyS (a constructor for the input-values FR):

λ rg "inReq EmptyS" -ths
drasil-example/glassbr/lib/Drasil/GlassBR/Requirements.hs
76:  S "from", refS (inReq EmptyS) `S.andThe` S "known", plural value,

drasil-example/swhs/lib/Drasil/SWHS/Requirements.hs
58:findMass = findMassConstruct (inReq EmptyS) (plural mass) iMods 
84:  [pluralNP (the value), fromSource (inReq EmptyS)],

drasil-example/swhsnopcm/lib/Drasil/SWHSNoPCM/Requirements.hs
31:findMass = findMassConstruct (inReq EmptyS) (phrase mass) [eBalanceOnWtr]
37:  [pluralNP (the value), fromSource (inReq EmptyS)],

We also hack in the unique descriptions of the input-values FR with FReqsSub:

λ rg " FReqsSub [^ E]" -ths drasil-example 
drasil-example/swhs/lib/Drasil/SWHS/Body.hs
153:    FReqsSub inReqDesc [],

drasil-example/swhsnopcm/lib/Drasil/SWHSNoPCM/Body.hs
147:    FReqsSub inReqDesc [],

drasil-example/glassbr/lib/Drasil/GlassBR/Body.hs
103:    FReqsSub inReqDesc funcReqsTables,

In the future, I figure that with the "correct encoding" of the System, we would be able to automatically remove the input-values FR in GamePhysics (which is meant to be a library (?)). For SSP, it looks like it had an input requirement that was not uniform with our other examples.

@balacij
Copy link
Collaborator Author

balacij commented Aug 29, 2025

By "conveyed through System", I mean through the "system kind" we're trying to figure out.

@balacij
Copy link
Collaborator Author

balacij commented Aug 29, 2025

Looking at our logs, we normally only have 1 new chunk found and placed in the ChunkDB: the "input-values" functional requirement. So, in this PR, I removed fillReqs in favour of manually placing the 1 FR chunk in all of our ChunkDBs. Unfortunately, not all of our examples currently had this generic input FR requirement, so it adds a "bad" FR to GamePhysics and SSP.

Another implicit issue here is that we put things in the ChunkDB and stable/ changed, but nothing in System changed. If FRs were placed in the System and only "grabbed" from there, stable/ wouldn't change.

Copy link
Owner

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

I say "Request changes" but really, I think this whole PR is too much of a step back. It was a worthwhile investigation, but I think the conclusion should be "this isn't the route forward".

-- | Fills in the requirements section of the system information using the document description.
fillReqs :: DocDesc -> System -> System
fillReqs [] si = si
fillReqs (ReqrmntSec (ReqsProg x):_) si@SI{_systemdb = db} = genReqs x -- This causes overwrites in the ChunkDB for all requirements.
Copy link
Owner

Choose a reason for hiding this comment

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

You're right that this PR seems exactly backwards, in the sense that filling the requirements part of the database via reading them from the requirements section of the SRS seems like the right thing to do! So all the duplicates should be because someone's inserting something by hand unnecessarily.

One process to build a system can indeed go through an SRS.

Copy link
Collaborator Author

@balacij balacij Aug 29, 2025

Choose a reason for hiding this comment

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

in the sense that filling the requirements part of the database via reading them from the requirements section of the SRS seems like the right thing to do! So all the duplicates should be because someone's inserting something by hand unnecessarily.

One process to build a system can indeed go through an SRS.

That's an interesting POV, and not what I expected at all. There's some real design to be done here. Removing the manually add ones from the ChunkDB and/or checking if they've already been inserted is counter-intuitive to me. Why do you prefer that over this approach?

I'll put out a new requirement for Drasil: I want to generate comments of the form "FR: Input-Values: Reads in the input values" automatically in the code. How should this happen in Drasil? Where should this knowledge come from? Where should the code generator get information that "FR: Input-Values" is related to the generation of the "inputs"-related code?

Copy link
Owner

Choose a reason for hiding this comment

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

Deep in a PR comment is not the optimal place for deep design discussions (knowledge will be lost). Wiki?

My POV: a human should state their desire (in this case for a requirement) once in the 'right' place, and then that knowledge should be propagated. Adding a requirement to an SRS seems like a reasonable human-adds-information move.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to #4352 to continue the discussion because moving to the wiki would involve another PR. I think it will be best to move to the wiki (and the code) after a bit more discussion.

$ insertAll tm
$ insertAll ci
$ insertAll cits
$ insert (inReq EmptyS)
Copy link
Owner

Choose a reason for hiding this comment

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

This is always a serious code smell. This embeds a bad assumption too deeply, i.e. will make gamephysics harder still.

@JacquesCarette
Copy link
Owner

Another implicit issue here is that we put things in the ChunkDB and stable/ changed, but nothing in System changed.

As far as I can tell, most of the changes are due to ordering changes, not more fundamental. So we seem to have an ordering sensitivity that should be fixed.

@balacij
Copy link
Collaborator Author

balacij commented Aug 29, 2025

As far as I can tell, most of the changes are due to ordering changes, not more fundamental. So we seem to have an ordering sensitivity that should be fixed.

Yep, but just to be clear, I was referring to the additions, not the reordering. I should've been more clear.

@balacij
Copy link
Collaborator Author

balacij commented Aug 29, 2025

I think this whole PR is too much of a step back. It was a worthwhile investigation, but I think the conclusion should be "this isn't the route forward".

Agreed! I'll leave it open while we discuss alternatives.

@JacquesCarette
Copy link
Owner

I was referring to the additions, not the reordering. I should've been more clear.

Can you give an explicit list? It's too hard to filter that out from all the other changes. You alluded to some in your text above, but I don't know if it is a complete list.

@balacij
Copy link
Collaborator Author

balacij commented Aug 29, 2025

Can you give an explicit list? It's too hard to filter that out from all the other changes. You alluded to some in your text above, but I don't know if it is a complete list.

For both SSP and GamePhysics:

  • One extra line in each of their .dot analyses (for the added input requirement)
  • One extra FR to their HTML, TeX, mdBook, and ipynb FRs section
  • One extra item in the traceability matrices of the HTML, TeX, mdBook, and ipynb

Everything else appears to be re-ordering.

@JacquesCarette
Copy link
Owner

For GamePhysics, that addition is just plain wrong. For SSP, it's likely a correct bug fix. Which should be fixed regardless of this particular PR.

@balacij balacij closed this Aug 31, 2025
@balacij balacij deleted the rmFillReqs branch August 31, 2025 03:59
@balacij
Copy link
Collaborator Author

balacij commented Aug 31, 2025

Moving discussion to #4352.

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.

fillReqss is responsible for the remaining within-table ChunkDB-related chunk overwrites

2 participants