-
Notifications
You must be signed in to change notification settings - Fork 66
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
Messy whole program structure #36
Comments
Hi Jose, the main structure of GPUSPH comes from a small simulator that was not even written in C; then ported to exploit the GPU with CUDA, then multi-GPU and multi-node. All of this with from a handful of main developers and a lot of small, precious contributions. Many contributions came by passionate researchers who do not have a background in professional software development and were often overwhelmed with tasks (you know how research works - one can't just work full-time "only" on maintenance for months). You're not the first one realizing that the overall structure has large improvement margins :-) but it is less trivial to realize that there are a lot of ideas, list of improvements, discussions and tests in the TODO list, not to mention side activities like modeling and interpreting experiments. What is lacking is not awareness or good will, but rather what in the software industry is called the "capacity" (aka: there are very few contributors with very few time available), while deadlines end up increasing the "technical debt". The approach currently used is the same used in many in the industry: rather than an abrupt rewrite, a long, incremental improvement process. You will find a lot of traces of it in the commit history. Every feedback is welcome but it is especially constructive when it ends with "I propose". Could you make a detailed proposal, possibly in form of code by forking the repository and implementing its main lines? I am sure the merge request will get all the attention it deserves. Thank you for your feedback! |
In addition to what @Narcolessico wrote, there are both historical and practical reason for the current design. And while we are steadily moving towards an improved internal architecture, this is still done within the constraints imposed by available time and workforce, which as mentioned isn't exactly abundant. Concerning specifically the current integrated relationship between user problems and main application, this is due to a choice made in finding an optimal compromise between development time and code performance. More in detail, the device code massively relies on templates to fence optional components (both code and variables), and avoid the mere support for additional features from having any impact at all on performance. The downside of this approach is that to support runtime selection of options requires the compilation of all (compatible) option combinations, which results in extremely long compile times. In fact, the choice to make the framework options a compile-time decision in the user problem came when, even with considerably less variety than we have today, it took several hours to come out with the independent main executable, which completely destroyed the performance of the edit/compile/run cycle during development. I can't say I'm happy with the decision, and as a matter of fact, we do have a solution planned, which consists in moving away from the current “single source” approach for the device code into a “separate source” approach, which is supported in recent versions of CUDA via NVRTC. Following up to @Narcolessico proposal to “put your money where your mouth is”, you could for example try and implement dynamic loading of specific instances of the device code via NVRTC, as a first step; this would subsequently allow the CUDASimFramework to be instantiated from within the GPUWorker, rather than the user problem, which is a prerequisite to untangle the user problem from the rest of GPUSPH. |
Hey guys! First of all, regarding netiquette... No offence, but impolite and not-diplomatic language is the generally accepted approach in free software development. It is, because polite discussions are not pragmatic, and don't let evaluate the level of concern. Now (since #31) you have a fully open project, so I would expect this short of comments quite often. Moving back to the issue itself...
Then publish the TODO list in the repo, and state in README how feature requests will be eventually addressed. Anyway, I would say that a bad structured code is a major issue... At least if you really want to get 3rd party developers contributing.
That's good! But AFAIK the required changes are not that great (for this specific problem). Of course, the "host side" can be easily adapted, as can be appreciated in main.cc:381 line (you just need to figure out how to reallocate the NetworkManager code). Regarding the "device side", using CMake (see #35) should not be hard to pack all the deps (revamping a bit the declarations) into a library. Further details here: https://devblogs.nvidia.com/building-cuda-applications-cmake/ Let me know! |
That is not really the case. Most of the discussion in and around free software development, even when not-diplomatic and straight-to-the-point is not impolite. If you are unable to communicate without coming off as arrogant and sanctimonious, then we can do without the communication, especially when you fail at being both polite and pragmatic —the only result of which is the time is spent in this kind of useless discussions.
So far you've been the only one, and you can expect anyone coming out as badly to be treated the same way.
These are things that will happen in time, as time permits and things evolve.
You'll find that there are several areas where contributions are possible despite the structural limitations of the current implementation.
You are looking at the situation from the wrong side up. A better build system is completely irrelevant with the current GPUSPH structure. The current GPUSPH structure (as in: the relationship between the GPUSPH class itself, Here's the pragmatic approach you can follow if you actually wish to contribute something useful instead of vague criticism from the peanut gallery:
If you manage to implement these preliminary steps fast enough, they could be merged for 5.0, which would allow us to obsolete the single-source framework for the next major release, allowing us to move forward in the refactoring for the libification of GPUSPH. |
Jajaja, that's great ;-) Maybe we can stop the weapon climbing at this point.
I consider that a major issue too... It is not I don't understand your point... I really do... But you cannot reply you have not time to work on something because a long todo list, without publishing such todo list... That would make people feel you just simply don't care... Recent similar situations here and here. BTW, you can add labels/tags to the issues. You know, like "feature request", "bug", "enhancement" and so on... Honestly, I don't do that in my own repos, but apparently people like it
I completely disagree! Wanna you get an extensible code by means of NVRTC? perfect! I like it! In fact AQUAgpusph works like that. But I really can't see what has it to do with getting a main + libs structure... I made AQUAgpusph use that paradigm much before going modular/extensible. For instance I consider
Directly came from main...
Can't this be librified?
Just attributes
Is this not a geometry library? I'm not as used as you to CUDA way of life, but are you sure you are not trying to make several changes in the same shot? |
Just a small remark on this point: there is no centralized TODO list AFAIK. There are TODOs in the code, some just orally discussed, some in shared documents. I would be useful to have them all together and prioritized, but at the time being there no single file to just push to the repo. |
Your are clearly not the first CFD libraries which is asking the user to code his own solver, e.g. Palabos, ASL, and so far OpenFOAM. However, the way GPUSPH is doing that is a mess...
In my opinion, GPUSPH shall be refactored in such a way that everything outside
src/problems
can be compiled as one or more libraries, and subsequently everything insidesrc/problems
can be linked linked against those libs.Right now, there is some sort of "main", which is basically managing everything to rely the control to the selected problem... As a consequence, each compiled problem is a whole new GPUSPH instance... From all the issues potentially associated to this behaviour, my favourite one is the error hiding.
The text was updated successfully, but these errors were encountered: