[FEATURE TRACKING] Preparing and Enhancing Open Source Development

[ganler]

Before open-sourcing the nnsmith project, I want to simplify and standardize a bit the development and user accessibility. Below is a tracking list of TODOs for @ganler to make the repository a better open-source one in the next few months.

• Setup pre-commit;
• Setup black formatting (within pre-commit);
• Improve code quality and make NNSmith extensible.
  • Sync with paper naming.
  • Modularize data in abstract domain (nnsmith/abstract);
  • Make model generation dependency-minimal and split it against model materialization;
  • Make operator constraints extensible for other model formats;
  • Refine backend runners to allow different model formats;
  • Operator dialect;
  • Add unit tests for serialization, op spec validity, etc.
  • Hydra configuration;
  • Fix dtype_test, fuzz and backend execution [X];
  • Allow single-step fuzzing (currently we do multi-step fuzzing for the sake of coverage evaluation);
• Setup Github CI; Thanks to @Co1lin!
• Setup pip package;

[ganler]

NNSmith new architecture for framework extension. cc: @Co1lin

[lazycal]

Might also consider dropping bitvector stuff and everything in nnsmith/abstract/arith.py (which is only for bitvector). I think Binning is a better way for attribute diversity than bitvector (more transparent unlike bitvector that solely relies on the constraint solver's algorithm which we may not know well). Another reason is that bitvector currently does not offer good randomness (though z3 is going to release a new feature that improves randomness, but still...). @fruffy feel free to let me know if you think otherwise.

[ganler]

Thanks for the heads up.

I am neutral on whether keeping bitvec or not. But we should drop it if we cannot maintain it. Talking about "arith", I think @fruffy previously did a good job on unifying operation for ArithRef and int with nnsmith_[op]. Because / for ArithRef can be integer division and for concrete integers in Python it creates floating-point numbers. I am wondering if we can make it more user-friendly in spec writing by using common operators like "+-*/%" etc, while overcoming the semantic divergence of division operator for ArithRef and Python int, without introducing extra Union[int, ArithRef] class wrappers that might make the type of shapes too complicated.

One workaround I can come up with is to extend the ArithRef class to substitute division "/" with floor div "//", forcing users to use "//".

[ganler]

Regarding attribute binning, it is definitely nice to have as long as we can make it as simple as changing a param in the command line and make its usage clear to the users. That said, enabling a new heuristic should not force user changes on the specification (but it's negotiable if it really brings strong benefits.) For example, our previous impl really scares me a bit...

nnsmith/nnsmith/graph_gen.py

Line 1305 in f95182b

PARAM_CONFIG1 = {

It will break things when users having a new abs. operator and certainly they usually don't want to modify things, let alone the source of our library.

Meanwhile, it is also very important to show ppl why and why not attribute binning. IMO attribute binning is not a silver bullet. Theoretically, it is only suitable for those that generate tons of different specialized types oeprators for extreme performance. Otherwise, it benefit on coverage is going to be subtle. And it lacks many merits the base method had: 1) those small values are naturally good edge cases; 2) it generates fast; 3) the algorithm itself is very simple to undertand and implement, etc. Again, I am not against having attribute binning. I just think there's still some work to be done before our bringing it to the stable release: we should make its integration "harmless" to our APIs and bring concrete examples and evidence showing that binnning is very useful for X framework/compiler with improvement of Y% coverage due the compiler's main feature of what, so that ppl know when to use it. I have not been working on it for now, as there are many other more important things to be solved, including the ones listed in the first post and bringing a more simplified constraint for model sizes (w/o asking everyone to implement flops + floats + etc for every operators)... I suggest we can work on attribute binning once those prioritized jobs are resolved.

[lazycal]

Your workaround with // sounds good to me.

For attribute binning, what you said are valuable research points, but my main problem really isn't at the research side, but something else (maybe Intellecture Property or whatever): IMO the paper is credited to all authors, so if this repo is going to be the official implementation of the paper, it should faithfully implement the paper. But as you said you did not have the plan to add it in the near future, so I previously asked if this is going to be just your own implementation. I think there can be three solutions to this:

1. Make it your own implementation.
2. Make it the official implementation, and add attribute binning someday (not asking to prioritize it, but just to make it scheduled in near future. I totally agree that the base method is the most important component that sets us apart from other works and we should make it well-formed). I am more than happy to help.
3. It looks like it is because you still have doubts on Binning that you do not plan to add it. Then let's discuss it, and conduct more investigation if necessary. We should also loop in everyone else too.

Item 3. is surely the most ideal one but of course would take more effort.

Reply to some points you wrote:

it is definitely nice to have as long as we can make it as simple as changing a param in the command line and make its usage clear to the users ...
It will break things when users having a new abs ...

It is true that specifying Binning constraints requires additional knowledge about the new operator. But I actually don't see why it is hard to make the process simple and non-code-breaking. For example, why not just not enable Binning for that operator if the user doesn't specify one, or more aggresively, use a default configuration (e.g., bins each attributes the same way)? This way isn't Binning "harmlessly" integrated?

1. those small values are naturally good edge cases;

Yes, but that is not transparent as it solely depends on the z3 algorithm to hit the edges. At Binning it is specifically enforced to generate small values (e.g., it has roughly 1/8 of probability to generate 1 if you have 8 bins).

[fruffy]

I am fine with dropping the bitvector stuff since we can not maintain it and there are few incentives to revive it. It is possible to get this implementation right with the correct tweaks and changes to the solver (i.e., not using Z3 which was not made for this or asking the Z3 maintainers directly on how to implement this) but the question is whether this is still something to pursue.

[ganler]

@lazycal Thanks for the clarification. I see your concern and I am sorry for not making it clear to you. I also understand your frustration for the current absence of your main contribution as the attribute binning. We should definitely properly address it.

IMO the paper is credited to all authors, so if this repo is going to be the official implementation of the paper, it should faithfully implement the paper.

I don't think anyone is against it and that's exactly something we need do to have a place putting code that "faithfully implement the paper". My proposal, also following how other ppl do, is to checkout to the submission version (d0d8ef8) and put it in another org (say "nnsmith-asplos-22"). We associate the repo with the paper by cross referencing with the paper.

I don't suggest putting ise-uiuc/nnsmith as a frozen archive for our paper as personally I wish to push it to a stable package and sustainably develop it with the open source. Open-source software evolution is inevitable. Holding a constraint that those systems must be frozen to the paper implementation version will largely limit its possibility. Therefore, I think my expectation conflicts with your hope to hold such constraints that even if I now say yes, at some point you will still have concerns when the project is moving forward to deprecate things. As a result, a safe way IMO is to have a seperate repo to represent the paper's implementation as what it is. According to my experience in AEC, many papers simply use a git tag/branch for artifact purposes, such as retiarii and sparta. That's why I assume that we can just follow them to put the artifact here and was not aware it can be an issue. But now I see it so let's use a full repo instead of a branch.

my main problem really isn't at the research side, but something else (maybe Intellecture Property or whatever)

Back to attribute binning, to make it clear, I am not opposed to it and I appreciate and am never interested in abating your contribution -- we also have all the "append-only" commit history. It is totally a technical suggestion that I hope we can make it to a stably working stage so that we have some good pure technical justification/evidence for it before shipping it to the public software toolchain. Otherwise, ppl might not understand why or why not to use it, or blaming us putting unstable / non-working stuff to them, which might compromise the project's popularity. I can surely make a concession to bringing it in immediately by taking the cost of tech debt, but I will be upset as it is not from a technical reason.

Back to the techinical discussion:

But I actually don't see why it is hard to make the process simple and non-code-breaking.

It does not seem to be hard. My comment for "breaking things" is only for the previous implementation, as I privately report the issue to you before, nnsmith will crash when adding new operators/attributes. It is also the primary reason I disabled it as it previously cannot work with operator extension.

Yes, but that is not transparent as it solely depends...

I do agree that having transparency to control the value ranges explicitly makes nnsmith more robust so that we don't have to rely on z3 in the future. At least we can have some easy options to set ranges of global variables which also alleviate generating OOM models.

To explain further, I never doubt attribute binning or any other solutions bringing "stable" randomness in the SMT solving would not contribute to "attribute diversity". The question is "how sensitive are existing frameworks to attribute diversity and can we find any instance of it?". For research we say it is beneficial to "a framework" that generates various specilized kernels. But if ppl from MLSys community is going to use it for fuzzing, it is quite important to let them know what the "a framework" is. Unfortunately, honestly we did not find the "a framework" yet as our experiements on ort / tvm did not bring clear coverage advance. So how can we justify it to the users? by saying it does not work for tvm and ort so far? It is also fine that we can just have it (but at least we should make it work) while telling ppl the truth and expecting someone someday figure out that it can be useful to X framework that we can encourage ppl fuzz X with attribute binning. But I am just not excited about that so it is not prioritized on my TODO list. Also, by "priority", I mean my personal priority. So I think generally we should let ppl work on and prioritize on things they are interested in given that they might have very compact schedules.

To sum up, I recommend having a sperate repo that "faithfully implements the paper" and we give pointers from the paper to the seperate repo. Meanwhile, if you are interested, you are still more than welcome to help realize attribute binning to a usable stage (or I can investigate later), (very) preferably with examples to let ppl know under what circumstances (i.e., which framework) we should use it.

[ganler]

the question is whether this is still something to pursue.

@fruffy I think we should definitely try it if we can make it without library modification. Also, thanks for the suggestion that I will try to ask z3's author if they have such plans.

[lazycal]

IMO the paper is credited to all authors, so if this repo is going to be the official implementation of the paper, it should faithfully implement the paper.
my main problem really isn't at the research side, but something else (maybe Intellecture Property or whatever)

Sorry I didn't make it clear. What I really wanted to say is: 1) We should have a place to put the code that faithfully implements the paper, regardless of whether it's in this main branch (that is what I meant by "the repo" in my prev message. I should've said "main branch") or a tag; 2) make it clear about the ownership of the main branch. If it belongs to everyone in the project then every change here (and the reason behind) will reflect the collective thought by default IMO. But if it is just your own implementation then it doesn't matter.

Now going back to this project, I never asked to freeze the repo or to have a separate one. I am happy with a tag. Also feel free to make the main branch personal. My problem is, your reason behind not including Binning contradicts with what is written in the paper. So we should make it clear if this is going to be your personal thought or the collective thought. For the latter we should use a discussion to get everyone on the same page about what to say about the contradiction with the paper, or change the paper to sync with the main branch.

I also understand your frustration for the current absence of your main contribution as the attribute binning

No as I said I agree base outweighs Binning quite a bit. The rest are less important things that I would not call them "main" contribution".

Unfortunately, honestly we did not find the "a framework" yet as our experiements on ort / tvm did not bring clear coverage advance. So how can we justify it to the users?

Regarding this research matter, why do we not use a discussion to address this? If it cannot even convince you, how can it convince reviewers? The rebuttal is about to start, and I think we can use this chance to start the discussion earlier to get the paper swapped into everyone's mind.

[ganler]

@lazycal Thanks for making things more clear. I feel it is better to alleviate your concern by having another repo (since also the "ise-uiuc" org might not directly reflect the contributions from other orgs) to fit the points you mentioned. That said we can create a repo checking out from d0d8ef8 which represents our paper officially (with cross-referencing) and belongs to every collaborator and everything there should "faithfully implement the paper" and "reflect the collective thought by default". Regarding the main branch here, IMO, you can simply assume it is going to be some personal implementation that might not "reflect the collective thought by default" as the software evolution is inevitable which may not seriously align with our paper.

My problem is, your reason behind not including Binning contradicts with what is written in the paper.

If it cannot even convince you, how can it convince reviewers?

Please don't get me wrong, my statements in #33 (comment) are 1) personal from a purely technical perspective; and 2) exactly something we had in the paper: as I said attribute binning is definitely helpful for "attribute diversity" and that's exactly where we show the results in S5.3. However, as is also shown in S5.3, the coverage improvement is very subtle for ort and tvm. It does not mean a binary judgment of good or bad. It simply means the coverage of tvm and ort are not that sensitive to "attribute diversity". Maybe I am a nerd, but personally, I think as a serious package it is important to at least justify the "a framework" so ppl know when to use it.

why do we not use a discussion to address this?

I am never against that and that's exactly something I'd hope. I am replying to you here simply because you started the conversation here. If you are worried that our technical discussion under this thread is going to hurt anything, I am fine to delete comments in this thread and move the discussion elsewhere.

Please don't hesitate to tell me your concerns if there is still any miss understanding. I sincerely want to properly get things addressed. Meanwhile, my comments so far are personal without discussion with my advisor, as we are busy with some recent deadlines in these two days that I don't want to distract his attention (tho I am trying my best to reply to you instantly). Thanks!

[ganler]

Second thought: if your exact worry is:

1. We have attribute binning in the paper;
2. We want to avoid criticism or doubts about it, esp. from the original authors;
3. It sounds like the original author has some doubts about the paper which makes things tricky;

Though point 3 is a complete misunderstanding of my comments, anyhow I think a good way to satisfy everyone, is to say let's work harder to find out "a framework" before the rebuttal or the camera-ready version. In this way, I have no problem with adopting attribute binning since we can say it is beneficial to X and it also makes the paper stronger as reviewers might attack us on its subtle coverage improvement, and we can directly show the evidence from the coverage respect instead of proving it with a less direct metric (e.g., #unique op instance).

I am happy to drive myself and work together to see if we can reach the goal. But it makes no sense if that is not where your primary worry is. Because, from your previous statements, it sounds like you are accusing me of abating your contribution or taking over your credits, as you said it's not a problem from the research side, and use serious words like "credits", "Intellectual Property" and "ownership", which I am totally not interested in at all, and I think the repo license and the commit history can justify those things. And I sincerely apologize if that's my misunderstanding.

That said, I honestly hope to catch your exact concerns so that we can work together to address them properly. I am also happy to chat with you in a more responsive fashion, say zoom. :)

[lazycal]

Second thought: if your exact worry is:...

This is the research side of my problem. As I said it is the most ideal solution if we work on this together to get on the same page.

Though point 3 is a complete misunderstanding of my comments

For 3. you can see my email. I think it is because we understand the paper differently which make us hard to understand each other.

find out "a framework"

Yes that sounds good, though I think from first principle most DNN frameworks are the "a framework". I think we can chat there and see how we can convince each other.

Because, from your previous statements, it sounds like you are accusing me of abating...

OK maybe it's because I don't know the repo license well, and I am sorry if that is the case. Also those are not serious words..., just words that describe the things most precisely, sorry if that made you think this way. It's also not about "abating my contribution" or whatever, but I was saying that we should make it clear that who are responsible for the statements made here. If you made some statements it sounds like it is our decision together and you are speaking for us, and it looks bad if someone actually doesn't agree with it. That is really my main problem, not just for Binning, but for other components too.