Independent Dev Testimonials

@dirvine

Discussion: WIP: feat(cli): implements 'safe reset' subcommand - Core Libraries - Safe Dev Forum

Original topic: SAFE CLI subcommand suggestions: reset and uninstall - General - Safe Dev Forum

5 Likes

Sorry @latch but I do think folk were only trying to help. Perhaps we need to get better at this part?

5 Likes

I think we all do. In recent months some big changes have been across repo’s and that has had a load of Engineers on slack trying to coordinate branches. So till launch we have been just merging to master and moving on quickly. It’s not the best but should calm right down now with also the 200 line limit on PRs. It’s all aimed now at reducing changes to testnet and beyond. Lot more CI checks going in as well now to catch incompatibilities so expect this part to really improve quickly now.

So good points guys, accurate and needing resolved.

9 Likes

@dirvine

I have no doubt everyone’s intentions were good however that’s irrelevant in my view.

I explicitly asked for guidance regarding the appropriate function for user confirmation. (SAFE CLI subcommand suggestions: reset and uninstall - #11 by latch - General - Safe Dev Forum) I didn’t receive a response so I created my own. Then the merge request was held up not for the feature I was adding, but to bike shed user confirmation. It wasn’t important enough to reply to originally, but it was important enough to hold up a contribution.

What should have happened is the merge accepted, user confirmation function bike shedding punted, and a forward issue created to track that.

2 Likes

I’ve yet to contribute to the safe network code base, but I did integrate with it in the past (years ago now, sadly). I’m not an expert in Rust, but I’ve been writing code in my day job for over 20 years, including Java (primarily), PHP and Javascript. Over the last 3.5 years, I have also been contracting, predominantly (but not exclusively) in Fintec. This has given me exposure to a lot of different applications, code bases, processes of varying quality and consistency.

I’ve recently started to look over the latest code base with renewed enthusiasm. I’ve read the Rust book and associated material and have started looking through the various Maidsafe repos. I’ve had a detailed look through sn_api (including sn_cli) and sn_client, tracing through authentication and sequences primarily. I’ve also started looking over sn_node and sn_routing.

My thoughts so far? In a nut shell, it is well organised and appears to be high quality code. There is good functional decomposition, well thought out abstractions (structs/objects) and as a result the code reads well. Given the degree of complexity and novelty, this is great to see.

On my travels I have seen far, far worse code. I have seen eye watering functions with hundreds of lines of code splatted within them. I have seen the absence of any structure beyond basic functional decomposition, with little in the way of types or abstractions. Some of this code was from institutions you would have thought (and hoped!) would have high standards. The Safe Network repos are a bastion of quality in comparison.

What would I improve? From what I have seen, unit testing isn’t a priority. There are unit tests, but they tend to be thinner on the ground than I would prefer. I understand that unit testing can slow down progress in the short term, but it has the opposite effect in the longer term. Personally, I tend to write more unit test code than application code and like a high level of coverage.

I understand that there are automated integration tests though (I haven’t seen these yet), which helps to bridge the unit testing gap though. As a newbie to Rust, I may also be overestimating how much unit testing needs to be done (relative to, say, Java). However, given the shape of the code looks ‘unit testable’, clearly the developers have a good understanding on how to separate concerns and use appropriate abstractions, etc.

Still, my journey into the safe code base continues. These are my honest opinions at this time though and I see much to be pleased about.

18 Likes

I don’t agree, I think it has been a priority for a long time. Each crate has many unit tests and this is what I meant by heavily tested codes. Unit tests are functions flagged with #[test] or #[tokio::test] attribute and for example sn_api has 235 of them and sn_node 25. Not bad IMO.

This is already true for some files. For example in sn_node, src/capacity/rate_limit.rs has 80 lines of application code and 200 lines of tests.

It’s only a matter of time to complete the set of tests functions if coverage is incomplete.

8 Likes

I haven’t reviewed everything. What I have reviewed of the latest versions I thought was well written and clean. The latest is much clearer than earlier pass throughs about 18 months ago.

The only recommendation I have at this point would be to add more comments. I know I am a bit biased on that point. I’m a sucker for comments porn… meaning that I tend to prefer code that is loaded with comments and tells a story along side the functions about what they do and how to use them.

8 Likes

That’s good to hear. I could find quite a few code files with no tests, but maybe that is my unfamiliarity with Rust. If there is good coverage and I’m just not seeing it, that’s great!

6 Likes

I just remembered something else that comfused me. The term chunk and blob appeared to be used interchangeably at times or in an inconsistent manner. I recall there was even something like a “chunkblobmap” or blobchunkstore. I would have needed more inline comments to guide me. I know @oetyng likes his blobs, but calling everything a chunk or making a much more clear distinction between the usage of chunk and blob would be helpful. Again, I haven’t read everything so you may have described this well somewhere that I missed. More inline comments will help to make sense of things when jumping in to the code at a random location.

5 Likes

We do have coverage results in the CI, so if you look at the actions you might see some or go to coveralls.io. I am not convinced we have as good coverage as we would like. In saying that rust is a wee bit different. I see it needs much less unit testing, but just as much integration testing as c++/c#/java. Reason for less unit testing IMO is the compiler forced correctness.

If you are from java/c++ then you generally write code, seems legit but won’t compile, you tweak about and it works, then a unit test shows a really stupid bug. Rust has this weird thing where you write code, compiler forces strict rules on you but guides you through fixes and voila, it works and woks correctly, sounds weird but it’s true.

Also rust has a lot of files for traits/errors etc. that are not easily unit tested (no logic).

For folk looking at comments (I agree, we need many more, especially in sn_node) if you type cargo doc --open then it will open a web page with the crate and all dependencies documentation. That’s pretty cool, also docs have tests in them that CI runs, that way if the documented examples go stale (on code change) CI fails.

9 Likes

I think semver should/could help us with this soon as well, once we have all crates released as v1.x

6 Likes

Great topic, some very good comments. Will likely be in more later, just one thing about this

These are distinct in the design doc, but that design is not implemented yet.
The naming is for a design where a blob is a set of 1-n chunks. In the vein of that design, it could also be renamed to a set.
If these were to mean the same thing I would prefer “chunk” over “blob” :slight_smile:

10 Likes

Just skimmed through all the files on the github. The code looks clean and well thought out, not everything is as self-evident however if you just look at what functions are being referenced it makes a lot more sense. Some commenting might help for outsiders but also add lots of additional space into the files and if you frequently work with the code it might be more of an obstruction than helping out to re-read the same comments scrolling through it.

I don’t know Rust but I do see it uses types everywhere which only makes code less error prone and act better in according what you expect it to do. Hope one day I can code like this too and it made me interested to learn the Rust coding language!

Some parts I saw comment with allow(dead_code), does this mean it is no longer used or that it will be removed?

Bottom line, this looks like quality code to me and shows the devs work with a complex concepts while maintaining separation of concerns. :clap: :+1:

@dirvine Is there a visual tool that can show relations in code? Like file x.rs has function a,b,c that y.rs uses etc. Would be very easy to see for someone not familiar with the code how everything relates and depends on each other.

8 Likes

Re tooling, I’ve reluctancy succumbed to VSCode which has excellent support for Rust and every other language I use (and probably better than commercial dedicated products).

This includes ways to find what uses particular functions, shortcuts to visit declaration, implementation etc. There’s a free community add-on for just about anything a programmer can think of, because so many did and then created it.

Some stunning stuff, including whole applications ported to run inside VSCode.

How did I miss this?

4 Likes

I use IntelliJ Rust as I have a license for my Java work. It lets you drill into function/method calls, including those in external dependencies where possible. This really helps to build up a visualisation of the flow in my head.

IntelliJ Rust in general seems decent, but I’ve not used it in anger yet. It nicely colour codes syntax and makes it easy to navigate though.

2 Likes

In VS Code you can also right click a symbol and click “browse references” which gives you a list of callers, etc. Not quite what you’re looking for, but you can hit Ctrl+t and tag on it from there and walk the rabbit hole yourself. Not quite as good, but still a feature I use quite often myself.

Thoughts on the Current Codebase

Now on to the “state of the code”, as told according to the opinion of one highly-fallible engineer. Please be gentle :slightly_smiling_face: I’m a little late to the party, but incoming brain dump.

TLDR;

The term “beta” is used and abused these days in all the wrong ways. When David says “beta”, I would say its smart to interpret it in the most literal sense possible. I take that to be that the major architectural pieces and algorithms are in place. It’s functional, but everything else is up for grabs still.

I do not expect that “beta” implies “nearly finished,”. Interpret it as closer to “minimum number of big features implemented” (not the same as “feature-complete”). I get the impression this is probably the “end of the beginning” or perhaps the “beginning of the middle”.

Accessibility

I’m going to comment on the current state of the code base through the lens of contribution accessibility. I think this reflects the level of maturity of a codebase well, since more mature code can devote more resources, all else equal, to finetuning, tweaking, and supporting community development efforts. I’ll break it up by crate for the crates I know about to some extent.

  • sn_api - Accessibility : 8/10
    • In my opinion the best entry-point into the codebase. Very well-documented (as it should be, since its front-facing for users) and architecture is simple/easy to figure out at a glance. As a result, it’s simple to find ways to improve the code or fix bugs.
  • sn_node : Accessibility: 3.5/10
    • Not for the faint-of-heart. You really need to get your hands dirty to figure out what’s going on. It generally feels like a construction zone still, and there are lots of TODO comments to be seen.
    • You better be up-to-date on the current state of the net algorithms to some degree before looking at this. The architecture is not tightly coupled, which is nice and flexible, but also makes it tedious at times to figure out call-chains unless you have an idea of where it might be going next already from prior knowledge.
      • This is especially if you aren’t familiar with this kind of heavy indirection in code. I imagine it’s even harder to grok if you’re used to traditional object-oriented programming in the context of something like web-dev.
    • Related to the above, the architecture isn’t the most straightforward, and it’s been changing a lot until recently (4500 line patch to remove some message wrapping logic was the last big one, which occurred only last week). According to David, this should calm down now, but historically it’s proved to be a challenge. That said, I still see lots of room for improvement in architecture, so I’m not sure how much that’ll hold true beyond the next test net.
      • Quick, rather technical example: mutably borrowing the whole Node for every node operation, regardless of whether it needs the whole thing or not, defeats the performance gains entirely of our async/await calls. This can’t be fixed trivially with an Arc<Mutex> because one message generates another message so you need to borrow the whole thing “just in case”.
    • Documentation is spotty, as mentioned, which doesn’t ease things. That’s the least of it though, because of the general effort required to contribute to this (e.g. understanding network algorithms & architecture), you can probably follow a few call chains to piece together any missing docs.
  • sn_routing & qp2p: Accessibility: 6/10
    • These are actually rather straightforward, but I’ve little experience contributing to them actively. The code looks clean enough and isn’t too hard to figure out. It suffers heaviest I think from the “how to contribute” problem outlined below.
  • I don’t know about ffi or some of the other stuff. I have a passing knowledge of BRB after browsing a bit, but not enough to comment on thoroughly either, but it seems to be among the most experimental code still.

Biggest Barrier to Contributors: HOW can I help?

Even if you’re comfortable enough to understand existing code and work with it, knowing HOW to contribute is still difficult. I find it’s difficult to know at a glance, without a lot of effort, to figure out what what needs to be done, which tasks are currently being worked on already, and which are still up for grabs for somebody to step in and help.

Example: There’s no great place to figure out what bugs need squashing, what areas of code need more tests, etc. This speaks to the fact that devs still seem concerned heavily with making things work more so than making sure there are breadcrumbs to follow.

The GitHub issues don’t help a ton, and are a hit or miss. Sometimes stale issues stick around for a while. Sometimes the issues are sort of non-specific and just general reminders. Sometimes code efforts aren’t tracked by an issue at all until its time to PR, which makes it easy to accidentally waste time on something that will be superseded soon by a patch (which is why it’s a good idea to run things first on the dev forum right now I think).

The easiest contributions are new features (e.g. a new cli command or something), since there’s fewer conflicts with existing code, and they exist outside of ongoing efforts. That said, new features are not always the most immediately useful, and make it hard for the community to help meaningfully in terms of things that will be relevant in the semi-near-future (e.g. tests, bugfixes, and refactors). Any bugfix/refactor/etc. I’ve personally submitted is the result of stumbling across it in the code base while doing something else entirely. It’s not for a lack of things to do, but a lack of the bandwidth needed to painstakingly document and triage them.

Conclusion

The code base is settling down, but it’s still not easy to contribute. Things have historically moved fast, and, while it’s getting slower, this indicates to me that I can hardly call the code “mature” either. There are a lot of unimplemented things still, even if the last of the bare-minimum functional pieces is almost in (referring to rewards).

I hope this doesn’t come off the wrong way. I’m just reflecting a bit on some things I’ve experienced or seen others experience along the way, and what that might indicate about the current state of “done-ness”.

18 Likes

IMO: PR author politely calls out bikeshedding as they saw it. Folk agree so PR is tested + reviewed, merged, and the rest dealt with down the line.


@latch you appeared to be engaged in that discussion you’ve now decided was bikeshedding…

There was an attempt there in that thread there was to get more input from more folk to start opening up such things to more contributions/reviewers etc. (There has been a sizeable uptick in contributions of late, which is great, but it’s also hard to stay up to speed with it all; so such things are going to be a necessity. Which I think is a good thing in terms of making folk feel more ownership of code as things start to stabilise).

If anyone feels a PR is getting bikesheddy, then the best route forward (I think… open to other ideas about how to address this) is to bring that up in the discussion. Bowing out with, “i dont have time” is fine. But it doesn’t do anything to highlight or address the issue. I was not aware you considered that to be bikeshedding until just now, eg. I had imagined you may come back to the issue in a week or so when you had more time.

I’m not sure anyone else there saw it as such either… (though I can see where you’re coming from; anyone on that thread can happily correct me if I’m wrong).

Perhaps there are better ways to address the bikeshed concept? Do folk have experience with other OS projects that deal with that in some fashion?

11 Likes

Just wanted to share that I agree with this statement. Sorry you felt that way @latch, I thought it was a pretty engaging discussion at the time. If it happens again, maybe just call it out if you’re comfortable with that? This community is pretty tight and I’m sure people would understand.

To be fair, I think it’s easy to come off as “bike-sheddy” inadvertently. Especially in engineering and especially when people care about the product. In this case though, I think it’s less about punting and more that the devs here all care deeply about the work and want to come to the best version of the code that’s possible. Having experienced this personally, in a large corporation where nobody can be asked to give a damn, being punted looks a little different than this IMO.

Sometimes it’s easy to get a little carried-away, sometimes code just doesn’t make it to master for one reason or another, and sometimes people come off as pedantic. But I don’t personally think, in the context of MaidSafe, that it’s because anybody here doesn’t want your help, or that they don’t appreciate the community effort (quite the opposite in fact!).

11 Likes

I haven’t re-read that PR discussion but have followed many here and elsewhere over the years and haven’t thought any of them were bikeshedding (which doesn’t mean that one wasn’t, because I haven’t gone back to look).

My observation is that almost all, and certainly the MaidSafe discussions are constructive and sensible. I often read them just to learn things including how to contribute to or maintain a shared codebase.

Having recently been on both sides, both of which are new to me I can see the difficulties. I both experienced the emotional side of being asked to make changes, and I think saw a contributor end up unhappy when I asked for changes as maintainer (though they haven’t said as much, so I’m left guessing).

In neither case do I think there was anything that should have been done or said differently, yet in both instances a contributor (me and probably a contributor to my code) had a difficult experience.

I am open to feedback, in case as maintainer I’ve missed opportunities to do better.

I didn’t feel the need to feedback to the maintainer wrt my PR (for wasm-git) because I don’t think there was anything he could have said better. But it was still hard to hear and respond when from his point of view he wanted and asked me to do more things before merging. I had not realised they would need doing and so was thinking my code would just be merged, me given a pat on the back and able to go and tackle the next thing on my list.

In the end I did the first change he asked for, or part of it, he helped so much that in fact it was really him. The second thing I didn’t want to do, and he accepted that, merged the code and left it as something which could be done later by someone else.

So I get both sides of this, and in particular how hard it can be for a contributor, particularly as someone new to this kind of collaboration like myself. As a maintainer, my experience as contributor meant I was trying really hard to be both clear and also not unreasonable, either in what I was asking our how I did so. Yet it still looks like that contributor was left feeling unhappy about the process. Although as noted I’m speculating because I haven’t receive feedback about how I handled this.

So @latch, I get you! This can be hard for contributor and maintainer. As contributors we can put a lot of effort into something, and then have to go through a process of others looking at something we think is ready and they raise things we didn’t anticipate, some we may think are unreasonable etc. That’s a vulnerable situation, and I’m not sure how I’m going to handle it myself when I do get a maintainer who’s not experienced, not clear, or just not socially skilled etc. I think what I will try to do is first be objective, and then decide if I think it’s worth me offering feedback. I might just walk away, if I don’t think it’s worth my time. It will also depend on how important my contributing is to me in a particular case.

So I think it’s great that you found a way to give feedback, and that as I’ve come to expect the response from MaidSafe developers is receptive even if they don’t necessarily agree.

One thing I know is that both sides of this are much trickier than I’d imagined! So I have a new respect for maintainers and contributors, and I’m even more amazed and more grateful for all that they do. Something important I’ve learned is that it helps to be on both sides, as maintainer of some things and contributor to others.

Well done and thank you all of you!

9 Likes

I could well believe that!

Writing unit tests for interpreted, loosely typed languages like PHP is definitely a pain in comparison to compiled, typed languages like Java. If Rust takes this a step further, there must definitely be fewer edge cases to consider.

I hope to find out for myself in a few weeks!

5 Likes