veg / tn93

TN93 fast distance calculator
MIT License
15 stars 7 forks source link

Create devcontainer.json #34

Closed niemasd closed 11 months ago

niemasd commented 11 months ago

With this devcontainer file, VS Code (and GitHub Codespaces) will be able to create the development environment automatically.

niemasd commented 11 months ago

Okay, the OpenMP edits have been included in the following commit:

https://github.com/veg/tn93/pull/34/commits/7a552381ac2ba0d522c380d47fefd25c867a84d2

I just tested that, starting from the development Docker image, I'm able to successfully compile and run tn93 as follows:

cmake .
make
./tn93 -h

However, I think this edit might break compilation in the Ubuntu environment. @stevenweaver Can you check?

EDIT: Actually, it seems like the "CMake on multiple platforms" check worked, so I think this is ready to merge!

EDIT 2: Never mind; it seems to break in the Ubuntu one. Is there a backwards-compatible syntax for this OpenMP syntax?

stevenweaver commented 11 months ago

@niemasd -- allow me some time to get to root of this issue. There seems to be a discrepancy between distributions that extends beyond which compiler version we are using.

niemasd commented 11 months ago

@stevenweaver Sounds great; thanks for looking into it!

stevenweaver commented 11 months ago

Hi Niema,

This is due to an incompatibility with OpenMP 4.5. Because OpenMP 4.5 standard is old, I don't think it's worthwhile to support it.

We can do the following to use gcc-10:

yum install oracle-epel-release-el8
yum install gcc-toolset-10
scl enable gcc-toolset-10 bash

Then proceed with CMake as normal.

Best, Steven

niemasd commented 11 months ago

Thanks for looking into that! Should we use gcc-10, or should we use the sed one-liner to edit that in the Docker environment?

stevenweaver commented 11 months ago

Well, what I'm worried about with the sed one-liner is that it would remove parallelism from some functions (if I'm reading this correctly), but I haven't checked what the impact on performance would be.

niemasd commented 11 months ago

Ah, I didn't realize that! I'll switch over to gcc-10, then. Better safe than sorry! Will create updated PRs ASAP

spond commented 11 months ago

Dear @niemasd and @stevenweaver,

I have some code in HyPhy to deal with different OPENMP versions. Take a look at https://github.com/veg/hyphy/blob/c1f1ca1ce0123bc7e20fac39810a7aa8ba26377b/src/core/matrix.cpp#L3987

Best, Sergei

niemasd commented 11 months ago

I reverted the OpenMP edits I had made to the tn93 source code, so it should now compile on the environments it used to, but it now won't compile in the Docker environment. Which of the following should I pursue?

  1. Incorporate @spond's HyPhy fix to get tn93 compiling in the Docker environment, or
  2. Incorporate @stevenweaver's GCC version update in the Docker environment

I would personally lean towards 2, but I am happy with what you two think is best

niemasd commented 11 months ago

I'll go ahead and move forward with option 2 (use @stevenweaver's GCC version update), and will create/update PRs accordingly.

niemasd commented 11 months ago

@stevenweaver The proposed fixes to use GCC 10 didn't seem to work. Specifically, I updated the Dockerfile as follows:

RUN yum -y update && \
    yum install -y cmake gcc-c++ gcc-toolset-10 git make oracle-epel-release-el8 && \
    scl enable gcc-toolset-10 bash

But I still get the following compilation error:

/workspaces/tn93/src/read_reducer.cpp: In function ‘void handle_a_sequence(StringBuffer&, StringBuffer&, Vector&, Vector&, long int, long int, Vector*)’:
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘firstSequenceLength’ is predetermined ‘shared’ for ‘shared’
             #pragma omp parallel for default(none) shared(currently_defined_clusters, try_cluster, sequence_lengths, current_sequence, current_clusters, firstSequenceLength, min_overlap)
                                                                                                                                                                                           ^
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘min_overlap’ is predetermined ‘shared’ for ‘shared’
make[2]: *** [CMakeFiles/readreduce.dir/build.make:76: CMakeFiles/readreduce.dir/src/read_reducer.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:129: CMakeFiles/readreduce.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

I'll investigate; any thoughts?

stevenweaver commented 11 months ago

@niemasd -- Do you still have the output of CMake? Is it picking up GCC-10? I used the Dockerfile provided with Visual Studio to confirm that it worked, but it was in an interactive terminal. I'll try running the updated Dockerfile as well.

niemasd commented 11 months ago

Here's the complete output; I don't seem to see the GCC version:

[root@codespaces-f9765f tn93]# cmake .
-- Configuring done
-- Generating done
-- Build files have been written to: /workspaces/tn93
[root@codespaces-f9765f tn93]# make
[  2%] Building CXX object CMakeFiles/nucfreqsfasta.dir/src/nuc_freqs_from_fasta.cpp.o
[  4%] Building CXX object CMakeFiles/nucfreqsfasta.dir/src/stringBuffer.cc.o
[  6%] Building CXX object CMakeFiles/nucfreqsfasta.dir/src/tn93_shared.cc.o
[  8%] Linking CXX executable nucfreqsfasta
[  8%] Built target nucfreqsfasta
[ 10%] Building CXX object CMakeFiles/readreduce.dir/src/read_reducer.cpp.o
/workspaces/tn93/src/read_reducer.cpp: In function ‘void handle_a_sequence(StringBuffer&, StringBuffer&, Vector&, Vector&, long int, long int, Vector*)’:
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘firstSequenceLength’ is predetermined ‘shared’ for ‘shared’
             #pragma omp parallel for default(none) shared(currently_defined_clusters, try_cluster, sequence_lengths, current_sequence, current_clusters, firstSequenceLength, min_overlap)
                                                                                                                                                                                           ^
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘min_overlap’ is predetermined ‘shared’ for ‘shared’
make[2]: *** [CMakeFiles/readreduce.dir/build.make:76: CMakeFiles/readreduce.dir/src/read_reducer.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:129: CMakeFiles/readreduce.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

At the command line, it seems like GCC-10 is indeed the version in the environment (but not sure if CMake just isn't using it)?

[root@codespaces-f9765f tn93]# g++ --version
g++ (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1.2.0.1)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[root@codespaces-f9765f tn93]# gcc --version
gcc (GCC) 10.3.1 20210422 (Red Hat 10.3.1-1.2.0.1)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
stevenweaver commented 11 months ago

Yeah, there must be cached CMake files somewhere, which is really weird if a fresh container is being generated from a Dockerfile.

niemasd commented 11 months ago

Ah yup, I was able to show the whole compilation commands as follows:

[root@codespaces-f9765f tn93]# cmake --build . -v
...
/usr/bin/c++ -DVERSION_NUMBER=\"v1.0.12\" -I/workspaces/tn93/src -fopenmp -O3 -std=c++14 -funroll-loops -fopenmp -MD -MT CMakeFiles/readreduce.dir/src/read_reducer.cpp.o -MF CMakeFiles/readreduce.dir/src/read_reducer.cpp.o.d -o CMakeFiles/readreduce.dir/src/read_reducer.cpp.o -c /workspaces/tn93/src/read_reducer.cpp
/workspaces/tn93/src/read_reducer.cpp: In function ‘void handle_a_sequence(StringBuffer&, StringBuffer&, Vector&, Vector&, long int, long int, Vector*)’:
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘firstSequenceLength’ is predetermined ‘shared’ for ‘shared’
             #pragma omp parallel for default(none) shared(currently_defined_clusters, try_cluster, sequence_lengths, current_sequence, current_clusters, firstSequenceLength, min_overlap)
                                                                                                                                                                                           ^
/workspaces/tn93/src/read_reducer.cpp:128:187: error: ‘min_overlap’ is predetermined ‘shared’ for ‘shared’
gmake[2]: *** [CMakeFiles/readreduce.dir/build.make:76: CMakeFiles/readreduce.dir/src/read_reducer.cpp.o] Error 1

Checking that /usr/bin/c++ command:

[root@codespaces-f9765f tn93]# /usr/bin/c++ -DVERSION_NUMBER=\"v1.0.12\" -v
Using built-in specs.
COLLECT_GCC=/usr/bin/c++
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --disable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
Thread model: posix
gcc version 8.5.0 20210514 (Red Hat 8.5.0-18.0.6) (GCC)

I'll look into seeing how to make sure CMake uses the c++ in PATH (which is the correct one), rather than using /usr/bin/c++:

[root@codespaces-f9765f tn93]# which g++
/opt/rh/gcc-toolset-10/root/usr/bin/g++
[root@codespaces-f9765f tn93]# which c++
/opt/rh/gcc-toolset-10/root/usr/bin/c++
niemasd commented 11 months ago

I think I figured it out: the scl enable gcc-toolset-10 bash command seems to only change the selected GCC in the currently running bash environment, not on all bash environments. Instead, I'll add it to the .bashrc in the tn93 Dockerfile so that it gets executed in every bash shell (and will source .bashrc in the Dockerfile so that downstream commands in the Dockerfile have it as well). Will follow up soon.

EDIT: I also changed it to the following command as per some comments from Googling:

source /opt/rh/gcc-toolset-10/enable
niemasd commented 11 months ago

@stevenweaver Confirmed that it worked! I've created a new PR with the fix: https://github.com/veg/tn93/pull/36