WIP: Improvements to make it easier to implement runners other than StandardT #804

ali-abrar · 2021-01-07T17:45:10Z

Refactor MonadThunk
Refactor Nix.Thunk.Basic
Add Nix.Thunk.Separate
Add Nix.Fresh.Stable
Add StableId
Remove Scopes from Context and replace with ScopeT
Remove the t parameter formerly representing thunks. It can now be inferred from m

TODO:

Provide implementations for citations class
Restore Nix.Lint
Fix 3 broken tests
Test thunk refactor more thoroughly (adding new tests as necessary)
Update Project design intro in accordance with Thunk changes

Anton-Latukha · 2021-01-07T21:13:00Z

Can you say, do your Thunk refactor theoretically/practically would be able to address/extend to in the future to receive the depth of provenance? There are: #215 #216 #217 #224 #777, which seems like a basic riffs theme everyone learned.

Anton-Latukha · 2021-01-07T21:18:47Z

Thought the next release would be small.

Seems like the next release would be quite a biggy )

Anton-Latukha · 2021-01-07T21:30:38Z

Nice thing you showed up with the open PR process.

If some parts would become MVP for merge - I am happy to do the multi-merge development process. Chokers are quite hard to merge from the both sides of the coin.

ali-abrar · 2021-01-07T21:40:19Z

Hi Anton. I'll have a look at the thunk issues you mentioned.

There are probably some parts of this that can be separated: perhaps the Context/ScopeT change and the StableId change could be separate PRs.

At the moment, I'm packaging up the example program I was testing these changes against. Hopefully that will help to clarify the reasoning behind some of these changes. I'm also happy to describe them in more detail than I have so far: I just wanted to get this up to get the process started.

I also noticed that README-design.md needs to be updated as a result of these changes.

layus

Quite a lot of stuff, with interesting bits flooded into the mass removal of the t parameter. The simplification of StandardT is a relief.

Now I am waiting for the next update :-)

layus · 2021-01-16T13:55:03Z

src/Nix/Builtins.hs

+        ref <- defer $ flip nvSet M.empty <$> buildMap
+        ([("builtins", ref)] ++) <$> topLevelBuiltins
+      let s = M.fromList lst
+  pushScope s currentScopes


This is not mere rewriting right. You just fixed the builtins.builtins == builtins issue here. I guess it should make it's way to the release notes ultimately.
@Anton-Latukha what do you think ?

Yes. Definitely, code looks like solves it.
Looked into the code and tried some. Would reserve giving any ideas or active code here, I need a couple of levels to my type programming game to be able to help or give meaningful ideas in the middle of such change.
From type, errors received it already looks pretty close to recurse on builtins in the type code. At this state it looks like Ali probably would just naturally arrive to replace builtinsBuiltin with self-referencing builtins.

Man, our code is terse. It needs or to understand it all togather with Nix internals, or much more documentation (I talk generally, not to Alis case).

layus · 2021-01-16T19:58:45Z

src/Nix/Effects.hs

+
+instance MonadStore m => MonadStore (ReaderT r m)
+deriving instance MonadStore m => MonadStore (ScopeT binding r m)
+instance MonadStore m => MonadStore (StateT s m)


I may be missing something here, but why are these instances needed ?
Would there be a way to make ScopeT binding r an instance of MonadTrans ?
None of the other monads in this file required this GeneralizedNewtypeDeriving + StandaloneDeriving. What is special about MonadStore ?

layus · 2021-01-16T20:01:27Z

src/Nix/Effects/Derivation.hs

 buildDerivationWithContext drvAttrs = do
    -- Parse name first, so we can add an informative frame
-    drvName     <- getAttr   "name"                      $ extractNixString >=> assertDrvStoreName
+    drvName       <- getAttr "name"                      $ extractNixString >=> assertDrvStoreName


Nit: The indent was correct, as the whole line is indented less. Not that important :-).

layus · 2021-01-16T20:02:41Z

src/Nix/Effects/Derivation.hs

      => Text -> m a -> (v -> WithStringContextT m a) -> WithStringContextT m a
    getAttrOr' n d f = case M.lookup n drvAttrs of
      Nothing -> lift d
      Just v  -> withFrame' Info (ErrorCall $ "While evaluating attribute '" ++ show n ++ "'") $
                   fromValue' v >>= f

+    getAttrOr :: forall v a. (MonadNix e f m, FromValue v m (NValue' f m (NValue f m)))
+      => Text -> a -> (v -> WithStringContextT m a) -> WithStringContextT m a


Poor you :-/. Having to write these hairy signatures is a pain.

layus · 2021-01-16T20:04:28Z

src/Nix/Effects/Derivation.hs

 {-# LANGUAGE TypeApplications #-}
-
+{-# LANGUAGE TypeFamilies #-}
+



Why are TypeFamilies suddenly required in a lot of places ? It there a simple explanation ?

I guess it is related to the t parameter being hidden away in the m one ?

layus · 2021-01-16T20:52:48Z

src/Nix/Thunk.hs

+instance MonadTransWrap (ReaderT s) where
+  liftWrap f a = do
+    env <- ask
+    lift $ f $ runReaderT a env


Am I correct that MonadTransWrap is a simplified MonadBaseControl ? Look very simmilar, with a bit less hassle.

layus · 2021-01-16T21:08:55Z

tests/TestCommon.hs

                      frames

-hnixEvalText :: Options -> Text -> IO (StdValue (StandardT (StdIdT IO)))
+hnixEvalText :: Options -> Text -> IO (NValue Identity (StandardT IO)) -- (StdValue (StandardT (FreshStableIdT IO)))


Reminder to drop the comments in the final iteration :-)

Anton-Latukha · 2021-02-06T23:02:19Z

A note about rebasing

BTW. I now probably have proper access to the PR branch. Because when we first did it, I was trying to rebase before I seen the email with the agreement.
I was careless at the end not only because of the time constraint, I kinda wanted to reach a hand as much as I can, but at the same time to punt to you to do a proper rebase in the end. Because I was under the impression that someone can need to learn to rebase properly. So I just not tested the result at the end and shot from the hip. But also I need to learn from that, to not give short time promises.

If it needs time, please from time to time keep the thread posted. I see you mentioned it.

If something - do not worry about desyncs, we are able to tackle that.

The strategy of tacking depends on the main strategy:

How do you estimate: PR "goes long", or "goes short?

If it goes short - you currently probably would still be able to figure out rebase yourself.
But soon the rebasing the PR to master in one jump would become horrible.
Git is quite bad at doing complex rebases in it.
In Git complex rebases is much better to do incrementally.

Big PRs need a lot of recursive rebasing. Learned it from epic PR of robust POSIX Nix installer that I kept in the most ideal for reading it like a guided tour when doing a review and ideal merge state for them for 2 years. I'm no longer salty about it. Just took a lot (a week or three) of git groom rebasing to prettify it into an ideal tour guide to merge. It is Nix #1565. To be honest, in 2 years they didn't fixed any of the reported bugs in the upstream version of the installer, so I needed to sync only one or twice in two years for something they drive-by hooked into the installer.

If PR goes long
then:

Since I know the old master, know the whole map (->) of what changes were introduced, and know the new master.
Since the changes I currently introduce are basic (map -> fmap, layout, equivalent lambda refactors, reorganizing things in modules and documenting them).
I can do backups as the very first thing, and then after something merged upstream - go and sync those changes into this PR.
And this time I would test them properly also ), so when you return to do work - we would have a current mergeable state in the PR to develop, to discuss, to check with CI and to merge.

Again, it is because the PRs of this size need frequent rebases, otherwise the complexity of the Git rebase increases.
Well, to be honest, one can make the rebase by the commit by commit, but that is the same thing that I propose.

Anyway, the any way you decide it would satisfy me.

* Refactor MonadThunk * Refactor Nix.Thunk.Basic * Add Nix.Thunk.Separate * Add Nix.Fresh.Stable * Add StableId * Remove Scopes from Context and replace with ScopeT * Remove the `t` parameter formerly representing thunks. It can now be inferred from `m` TODO: * [ ] Provide implementations for citations class * [ ] Restore Nix.Lint * [ ] Fix 3 broken tests

Anton-Latukha · 2021-02-18T16:15:42Z

Another one.

The previous version is at: https://github.com/haskell-nix/hnix/tree/2021-02-18-2-backup-aa/monadthunk-experiment

Anton-Latukha · 2021-02-27T16:08:11Z

Well. master & PR sync gets out of hand.

I can not stop the work in the project, because of this PR.

I do treewide pretty trivial refactors to the source code, doing clean-up, organization, optimization & other stuff.

If to rebase - the master changes in the code are documented in the commits & in the ChangeLog.

After the work done in the PR, it would be easier to take the resulting PR diff, and submit it over the new master code, for which the master triviality and documentation of changes helps tremendously.

Also after refactors in master - the logic and the work on it would become much easier. If you would need Kliesly HOFs - I preserve them.

Thank you also very much, HNix definitely needs this work allowing multithreading.

Anton-Latukha · 2021-03-07T10:55:33Z

News:

Think you would find the updated implementation of Standart easier to work with.

Over the project, the code complexity that relates to your work was reduced a bit and received help.

Code now allows equivalently using both normal and old Kleisli-aware implementations that are now *F.
So you allowed to mix and match them both during development however it would be simpler to you to simplify the process.

Also new MonadValue StdValue function signatures which are annotated, simplified, and straight-forward - should help in development, and if somewhere would be difficult or customization of implementation would help - there are isomorphic old ways for it.

Also in the shortest time, the Nix.Value module would go through the same scenario (ordering the arguments into classical positions, the formation of Kleisli into *F``), and by that our basic NValue{F,',}` types would receive lawful instances for type classes: #776.

After that work, renewing, troubleshooting, and merging of the patch should be a relative breeze.

Sorry for breaking backward compatibility with the current patch code by giving more convenience.

Anton-Latukha · 2021-08-13T12:29:43Z

@ali-abrar.

master branch desynced with this PR largely because the two-side communication (& so the merge process) stopped, your last message was #804 (comment) on Jan 7, after that layus made a review, I rebased & messaged for ~1.5 months in the thread and got no messages back, so what maintainer can do in that case ¯\_(ツ)_/¯.

Currently, meanwhile from the last post, HNix went through a pretty big refactoring phase.

Сode is cleaned-up & better organized, & more documented, which should simplify the work & understanding of things during work.

The refactoring phase I was doing is done. Currently, I can stop making changes for a month or two, especially to make things happen or merge something like this, we are also precisely at a phase that: what API breakages are needed - they can be merged without remorse.

I am also personally interested in this work. Please, if you want to finish - you can, but write to me so we coordinate efforts. Or would ask at least to supply comprehensive information collected, the idea of it & implementation, on the scopes, provenance & "fresh" thunking (the reason behind fresh is still a kind of mystery not only to me but now for John himself also, it seemed to relate to the evaluation/thunk initialization). As I understood the ScopeT & thunk mechanism changes are central pieces.

ali-abrar changed the title ~~Improvements to make it easier to implement runners other than StandardT~~ WIP: Improvements to make it easier to implement runners other than StandardT Jan 7, 2021

ali-abrar added the API change Requires major release. label Jan 7, 2021

ali-abrar mentioned this pull request Jan 7, 2021

WIP: An alternative implementation of thunks #481

Closed

Anton-Latukha mentioned this pull request Jan 7, 2021

Children finding their parents #805

Merged

ali-abrar force-pushed the aa/monadthunk-experiment branch 2 times, most recently from 741f43b to a474d77 Compare January 8, 2021 22:15

haskell-nix deleted a comment from ali-abrar Jan 9, 2021