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

md5sum regression #31

Closed
stevegt opened this issue Dec 7, 2020 · 10 comments
Closed

md5sum regression #31

stevegt opened this issue Dec 7, 2020 · 10 comments

Comments

@stevegt
Copy link
Contributor

stevegt commented Dec 7, 2020

Hey Jason, we were pretty happy when we saw you merge #30 -- thanks. We put a lot of work into that tool, with days of testing and debate over implementation. In case you haven't figured it out yet, you essentially have a team of paid devs available to you right now. The way we see this, you've got your head wrapped around the low-level geometry and sdf math; our best contribution, with a couple of us having decades of dev, sysadmin, and test harness experience in multiple languages and platforms, is testing, error handling, API use cases, and other package infrastructure. I think sdfx can go pretty far if we team up on this right.

For instance, I could have written a Python or Bash version pretty quickly, but we wrote the md5sum tool in Go so it would be more portable. Going back to Python doesn't seem right for a Go package. There are some things I might do differently in md5tool/main.go myself, but balancing my own preferences versus those of team members is something I have to consider as well; I had decided to defer md5tool cleanup until later commits.

For another example, the new ./mk/example.mk doesn't work on a Mac -- @koshchei is on a Mac, for instance. We used Go's md5 algo, because on a Mac the CLI command is named md5 rather than md5sum, and the output is radically different than Linux.

So can you pretty please revert this weekend's changes, looks like mostly ac530c6? If you need me to do it, I'd be happy to do the grunt work.

I'd keep the common example.mk -- we were thinking of doing something like that, probably putting it under examples/example.mk, but decided not to tackle that on Friday because we are trying not to overwhelm you with pull requests. We had only gotten as far as thinking of using symlinks; I like your include method better, again because it will be more cross-platform.

Thanks again,

Steve

@deadsy
Copy link
Owner

deadsy commented Dec 8, 2020

Yeah - I thought that might be a pain point :-)

Having a go based tool for the md5 hash stuff is like using a sledge hammer to kill an ant.
Given the portability concerns I'd suggest just modding mdtool.py to do both the update of the hashes and the check.
Then it should work on a mac ok.

I notice they put an older version of gnu make on the mac I have around (but clearly never develop on...) so perhaps an upgrade is in order to make the include stuff work properly.

Here's what I use:

$ make -v
GNU Make 4.2.1

@deadsy
Copy link
Owner

deadsy commented Dec 8, 2020

btw - curiosity lead me to do a build on a raspberrypi-4 (quad core 64-bit arm)
The md5s don't match. Not sure why - IEE754 implementations may not be as consistent as all that.
Looking at the spiral.dxf (arm64 vs amd64) the floats are typically different around the 12th decimal place.

@stevegt
Copy link
Contributor Author

stevegt commented Dec 8, 2020

The Raspberry Pi results are a good check -- I should have done that; we have a lot of those in our infrastructure. So it sounds like the md5 checks are more of a short-term stopgap in the first place, and we're going to have to work on real test cases.

Yeah - I thought that might be a pain point :-)

The duplicate work has caused some morale problems on my end. We're a family-owned business with several employees who in turn have a total of around 20 family members dependent on us. Because we supply to the labs, aerospace, and such we're not doing as badly as most right now, but we're not out of the woods yet either. We expanded our dev team over the summer, pulling in folks with a range of experience in order to help support folks while working on our own recovery.

Everyone is having to choose which wagon to hitch their future to right now -- for our younger members, it's about balancing their class load against building their resume while earning an income. I've been doing what I can to show them how to interoperate with larger and open-source projects, including good stewardship and community building. For our more experienced folks, it's about balancing family and life planning against career interests -- aside from earning an income, doing something that will still matter years down the road is important to them too.

We've all chosen to spend most of the last couple of weeks working on sdfx. I think this has been worth it, not only because we can use it ourselves but because we think it's a worthy cause. I think you've started something that has the potential to more than take the place of OpenSCAD over the next 10-20 years.

Having a go based tool for the md5 hash stuff is like using a sledge hammer to kill an ant.

I'd have a hard time defending that viewpoint with folks here. What you're essentially saying is that I'd need to convince Go devs with years of experience that they need to maintain Python code and a 200 Mb runtime. Using Python as the test cases for a Go package is not idiomatic, etc. Even I had to learn that lesson after seeing my own PRs rejected for using bash or python for driving Go test cases in other projects.

Don't get me wrong -- I was a Python guy for two decades, and even had the pleasure of inviting Guido to speak at the Silicon Valley Linux Users' Group after he moved to California in the early 2000's. A major reason I'm migrating us to Go these days is that, in comparison to Go, the runtime size and maintenance of Python installations, even using something like pyenv or venv, isn't worth the cost and infrastructure tooling complexity when compared with Go's deployment and package management, particularly after Go's modules were introduced.

I want us to be able to pull from your master branch, which means it needs to be able to pass make test on all of our machines -- maintaining our own sdfx fork is exactly what I'm trying to avoid. Rephrasing my earlier question from #22 (comment) -- are you interested in scaling sdfx to a larger community? If so, I'm all in. If you'd rather keep it more limited, then just let me know -- if that's the case, the thing we should be working toward here, I think, is a clean API such that we can wrap it with a larger project, adding our own test cases as needed if you're not up for deep coverage etc.

@deadsy
Copy link
Owner

deadsy commented Dec 9, 2020

I want us to be able to pull from your master branch, which means it needs to be able to pass make test on all of our machines

go + gnu make + base python3

Those shouldn't be a problem on any machine worth developing on. I don't know what's going on with Windows these days, but Mac shouldn't be a problem. I understand some web developers like to use it. Frankly I like python as hackable scripting language. Maybe I should use scons instead of make :-)

In general though - if idiomatic is your thing, test cases should be in *_test.go.

Now you could do the md5 thing as a part of that, but it's actually pretty useful as it is. You may have noticed I split out the rendering code into it's own package the other day. Having the md5s match indicated that I hadn't messed it up.

@deadsy
Copy link
Owner

deadsy commented Dec 10, 2020

Please retest on mac. I switched to sha1 (using shasum) and it works on my mac.

@mrsimicsak
Copy link

There are known inconsistencies between floating point implementations, I know its a problem for multiplayer game developers, where using floating point for coordinates leads to players getting out of sync with each other.

I suspect it is simply a matter of time before any test based on hashing the results of floating point calculations will work on one computer but not another.

It might be beneficial to split the rendering engine(s) tests from the sdf calculations tests. Then you could test the sdf calculations by replacing the rendering code with something the samples the sdf(s) on a 3 dimensional grid and compares the samples to a known good set. Maybe take an average of the delta or the squares of the deltas between the samples and the known good set and set a threshold on how different they are allowed to be? That would allow some wiggle room while still catching the failures.

I'm wondering if the best way to test the rendering engine isn't simply to provide a reference object and the object created by the test and ask a human to verify they are the same.

@deadsy
Copy link
Owner

deadsy commented Dec 11, 2020

From what I've seen (3 samples) the output is tracking the CPU vendor. ie Intel vs ARM. I'd be curious if an AMD chip had different results given they probably have their own FPU. As it is the hashes are still useful. ie - they facilitate regression testing. They also are fast, use very little storage and don't need any "is it close enough" type comparison of floats. You just have to be running it on an Intel CPU.

@stevegt
Copy link
Contributor Author

stevegt commented Dec 13, 2020

It might be beneficial to split the rendering engine(s) tests from the sdf calculations tests. Then you could test the sdf calculations by replacing the rendering code with something the samples the sdf(s) on a 3 dimensional grid and compares the samples to a known good set.

This is the right long-term approach, I think.

I'm wondering if the best way to test the rendering engine isn't simply to provide a reference object and the object created by the test and ask a human to verify they are the same.

We've been using the Hausdorff Distance filter built into meshlab for that -- works, and has helped us verify those cases where the md5 does differ. I see some Go implementations that might be able to be adapted for sdfx: https://github.com/search?q=language%3AGo+hausdorff&type=Code

@deadsy
Copy link
Owner

deadsy commented Dec 15, 2020

Per platform file hashes is a simple way of getting most of the way there. Once ARM on the Mac becomes a common thing then I suppose there would be a need for it.

@stevegt stevegt mentioned this issue Dec 24, 2020
@deadsy
Copy link
Owner

deadsy commented May 4, 2022

The SHA1SUM files are a good regression test. They won't be consistent on ARM based machines but that's not a super important use case for me. Someone using an ARM based MAC can maybe add some alternative SHAs that work ok on such machines.

@deadsy deadsy closed this as completed May 4, 2022
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

No branches or pull requests

3 participants