search

lamikr / rocm_sdk_builder

Other
113 stars 8 forks source link

Improve variables formatting #47

Closed daniandtheweb closed 2 weeks ago

daniandtheweb commented 3 weeks ago

Some little formatting fixes for the variables.

daniandtheweb commented 3 weeks ago

At first I changed it because I wanted to keep the envs loaded in my zshrc and having a default python different from the system's one can cause some issue when building packages. However that little change requires way too much refactoring around the project so I just reverted it.

lamikr commented 3 weeks ago

No, the purpose definetly is to have own version of python environment. Otherwise there would come just too easily all kind of distro and python version specific package conflicts where building fails. I had in that way long time and it was always some distro-version which got broken but since starting to use own python-version most common packages have just worked.

If it starts feeling too old maybe updating later to 3.10 or 3.11 but now 3.9 seems to be very stable and still well supported.

lamikr commented 3 weeks ago

Btw, is there some advantage of using the "$TEST" syntax instead of ${TEST} syntax in bash. I have liked to use braces because then it's easy to detect the variables when reading the code.

daniandtheweb commented 3 weeks ago

I just tried following the standard: no braces or quotation marks when the variable is a hardcoded string, braces for variables when they're followed by a hardcoded string and quotation marks whenever the content of the variable isn't only a hardcoded string.

daniandtheweb commented 3 weeks ago

The last one it's for example: "${ROCM_HOME}/bin:$PATH" PATH doesn't have anything as it's not with something hardcoded, ROCM_HOME is between braces as it's followed by /bin and everything is between quotation marks as it's not just a hardcoded string.

lamikr commented 2 weeks ago

I have just been bited couple of times with some strange problems when using quotes and I am not sure whether it's been my fault by properly escaping the special characters in a way that I have missed some or used by extra ones around another quotas. Or it could have been even a bug in cmake, hipcc, clang or linker's way of parsing parameters with quotas (Seeing some bug reports from those also in upstream) so that's way I have adopted using the braces around symbols.

You seem to be pretty good in bash and I started to think about and idea of refactoring the binfo_list.sh in a way that it would only have a list of apps to build without bash syntax and then creating the LIST_BINFO_FILE_BASENAME list dynamically. Reason for this is that I was thinking that it would be nice to be able to specify additional build lists for babs.sh as a parameter.

So there could be a "core" feature list that is always build but then some extra apps like ollala or whisper could be in their own extra app list.

daniandtheweb commented 2 weeks ago

That seems like a cool idea, machine learning projects on AMD are usually harder to build or at least tend to have more problems than cuda so having build scripts inside the projects could help a lot.

daniandtheweb commented 2 weeks ago

I'm quite sure this change shouldn't cause any issues, however if you think that they could cause issues I can just close the PR or keep it idle for now. I'll take a look when I have time to the main scripts to check if there may be any parsing issue related to those variables.

lamikr commented 2 weeks ago

Yes, I would like to keep this on hold or start accepting the changes in small peaces.

But it's good if you have time check howto do the binfo_list.sh change we talked. I was thinking that maybe "./babs.sh -b -f binfo/feature.binfo_list" or something could be the invoke command.

Btw, I just pushed for testing the wip/rocm_sdk_builder_612 branch for testing. It builded on my laptop ok on Mageia 9, needs to check other distros later.