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

Minor build tweaks #233

Merged
merged 3 commits into from
Jan 14, 2025
Merged

Minor build tweaks #233

merged 3 commits into from
Jan 14, 2025

Conversation

jwallwork23
Copy link
Contributor

This PR:

  • Gives some information on additional build dependencies in the README. In particular that torchvision should be installed at the same time as torch.
  • Drop FATAL_ERROR from CMake versioning, which is ignored.

@jwallwork23 jwallwork23 added the documentation Improvements or additions to documentation label Jan 13, 2025
@jwallwork23 jwallwork23 self-assigned this Jan 13, 2025
@ElliottKasoar
Copy link
Contributor

ElliottKasoar commented Jan 13, 2025

Do you think it might be worth bumping the minimum CMake version to 3.18, as they have in PyTorch?

I think 3.15 still works in most cases, but CMake 3.18 was the first version to include support for any compiler, enabling CMAKE_CUDA_STANDARD 17 support (it also includes potentially useful things like CMAKE_CUDA_ARCHITECTURES), so it may be safer, although C++17 is already in the FTorch requirements separately.

@jatkinson1000
Copy link
Member

PyTorch generally suggests installing torch, torchvision, and torchaudio as a oneshot.
Whilst looking at the conda side of things I feel we do not need torchaudio and torchvision is only a dependency for resnet in example 2, though should check this to be absolutely sure.

I'm still on the fence about FATAL ERROR and whether, given it has no effect beyond CMake >= 2.6, it should be removed.
The docs state that it "should" be included in case anyone tries building with a version lower than 2.6 (unlikely but a possibility). My reading of the CMake docs, though I have not tried doing this, is that if someone tried to build FTorch using an older version of CMake it would not raise an error about the version and instead fail elsewhere in the script.

@henryiii
Copy link

henryiii commented Jan 13, 2025

I haven't seen CMake <2.8 in many years. CMake 2.6 came out in 2008. I would not worry about what happens if someone tries to run with 2.6; nothing else will work either, it likely wouldn't even run on modern systems.

This is the equivalent of worrying about what error message is printed if a user runs Python 2.5. (Python 2.6 and CMake 2.6 came out the same year).

@matthewfeickert
Copy link
Contributor

matthewfeickert commented Jan 13, 2025

(I deleted my previous message as I now understand you were concerned about the effect of CMake v2.x which would print message but keep trying to build, ignoring the cmake_minimum_required minimum version, instead of failing out before the build. Sorry for misunderstanding.)

I agree with Henry that I wouldn't worry about people trying to build with CMake v2.X. Do you know of people you work with that are still using CMake v2?

Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Both, and @jwallwork23
In that case I'll trust your judgment, and I also did a quick scan of linux distros and their requirements and concluded anyone using anything old enough to come with CMake 2.x probably has bigger concerns!!

So I'm happy to approve this for merge.

@jwallwork23 jwallwork23 merged commit 542255e into main Jan 14, 2025
5 checks passed
@jwallwork23
Copy link
Contributor Author

Do you think it might be worth bumping the minimum CMake version to 3.18, as they have in PyTorch?

I think 3.15 still works in most cases, but CMake 3.18 was the first version to include support for any compiler, enabling CMAKE_CUDA_STANDARD 17 support (it also includes potentially useful things like CMAKE_CUDA_ARCHITECTURES), so it may be safer, although C++17 is already in the FTorch requirements separately.

@ElliottKasoar Thanks for the suggestion. I opened this as a new issue: #234.

@jwallwork23 jwallwork23 deleted the minor-build-tweaks branch January 14, 2025 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants