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.

Sign up for GitHub

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

Jump to bottom

Remove flake8 suppressions for invalid escape sequences #20161

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Remove flake8 suppressions for invalid escape sequences #20161

merged 1 commit into from
Mar 21, 2024

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Mar 20, 2024 •
edited
Loading

Those will become a SyntaxError in some Python version, so that suppression to make CI pass isn't a good idea, because that sequence IS invalid, no matter what flake8 says.

Solution is either to use raw strings or remove the escapes.

For OmegaFold it isn't required as an escaped space is used just to make for nice indentation which isn't required at this place.

FYI: ReaxFF-2.0-GCC-11.3.0-sim.eb requires a registration to download the source. However in that file only the comment was removed, so a rebuild isn't exactly required to check

@Flamefire
Remove flake8 suppressions for invalid escape sequences
298941a 
Those will become a SyntaxError in some Python version,
so that suppression to make CI pass isn't a good idea,
because that sequence IS invalid, no matter what flake8 says.
Solution is either to use raw strings or remove the escapes.
For OmegaFold it isn't required as an escaped space is used just to make
for nice indentation which isn't required at this place.
@casparvl
Copy link
Contributor

@boegelbot please test @ generoso

@boegelbot
Copy link
Collaborator

@casparvl: Request for testing this PR well received on login1

PR test command 'EB_PR=20161 EB_ARGS= EB_CONTAINER= EB_REPO=easybuild-easyconfigs /opt/software/slurm/bin/sbatch --job-name test_PR_20161 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 13156

Test results coming soon (I hope)...

- notification for comment with ID 2009804452 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
FAILED
Build succeeded for 2 out of 3 (3 easyconfigs in total)
cns4 - Linux Rocky Linux 8.9, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/boegelbot/e8f009e94e970c5a7c047e9da2ac7bf5 for a full test report.

@casparvl
Copy link
Contributor

Ok, that failure was expected, as ReaxFF needs to be downloaded by hand. I'll consider it a pass :) I'm building the other two on my own system as well.

@casparvl
Copy link
Contributor

@boegelbot please test @ jsc-zen3
EB_ARGS="Delft3D-4.04.01-foss-2022a-FLOW.eb OmegaFold-1.1.0-foss-2022a-CUDA-11.7.0.eb"

@boegelbot
Copy link
Collaborator

@casparvl: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de

PR test command 'if [[ develop != 'develop' ]]; then EB_BRANCH=develop ./easybuild_develop.sh 2> /dev/null 1>&2; EB_PREFIX=/home/boegelbot/easybuild/develop source init_env_easybuild_develop.sh; fi; EB_PR=20161 EB_ARGS="Delft3D-4.04.01-foss-2022a-FLOW.eb OmegaFold-1.1.0-foss-2022a-CUDA-11.7.0.eb" EB_CONTAINER= EB_REPO=easybuild-easyconfigs EB_BRANCH=develop /opt/software/slurm/bin/sbatch --job-name test_PR_20161 --ntasks=8 ~/boegelbot/eb_from_pr_upload_jsc-zen3.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 3817

Test results coming soon (I hope)...

- notification for comment with ID 2009905979 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
FAILED
Build succeeded for 1 out of 4 (2 easyconfigs in total)
jsczen3c1.int.jsc-zen3.fz-juelich.de - Linux Rocky Linux 9.3, x86_64, AMD EPYC-Milan Processor (zen3), Python 3.9.18
See https://gist.github.com/boegelbot/1e949552808a27e8b9411b679f758190 for a full test report.

@casparvl
Copy link
Contributor

Test report by @casparvl
FAILED
Build succeeded for 7 out of 10 (2 easyconfigs in total)
tcn1.local.snellius.surf.nl - Linux RHEL 8.6, x86_64, AMD EPYC 7H12 64-Core Processor, Python 3.6.8
See https://gist.github.com/casparvl/442da312be561af0f4dd80ac2b83da42 for a full test report.

@Flamefire
Copy link
Contributor Author

@casparvl All your issues seem to be with SuiteSparse trying to write to the wrong folder:

file INSTALL cannot copy file
"/gpfs/nvme1/1/casparl/ebbuildpath/SuiteSparse/5.13.0/foss-2022a-METIS-5.1.0/SuiteSparse-5.13.0/GraphBLAS/build/libgraphblas.so.7.2.0"
to "/usr/local/lib64/libgraphblas.so.7.2.0": Permission denied.

And PyTorch fails due to

flexible array member ethtool_link_settings

Which is #19746

Not sure if we should do much about this?

@casparvl
Copy link
Contributor

casparvl commented Mar 21, 2024 •
edited
Loading

file INSTALL cannot copy file
"/gpfs/nvme1/1/casparl/ebbuildpath/SuiteSparse/5.13.0/foss-2022a-METIS-5.1.0/SuiteSparse-5.13.0/GraphBLAS/build/libgraphblas.so.7.2.0"
to "/usr/local/lib64/libgraphblas.so.7.2.0": Permission denied.

Seems extremely weird no? Why would it be installing into my system prefix?

Edit: checking the build command, it seems completely correct... no clue why the CMAKE_INSTALL_PREFIX is ignored

( cd GraphBLAS && make JOBS=8 CMAKE_OPTIONS='-DCMAKE_INSTALL_PREFIX=/scratch-nvme/1/casparl/generic/software/SuiteSparse/5.13.0-foss-2022a-METIS-5.1.0 -DCMAKE_CXX_COMPILER=g++ -DCMAKE_C_COMPILER=gcc' install )
make[1]: Entering directory '/gpfs/nvme1/1/casparl/ebbuildpath/SuiteSparse/5.13.0/foss-2022a-METIS-5.1.0/SuiteSparse-5.13.0/GraphBLAS'
( cd build ; make install )
...
Install the project...
-- Install configuration: "Release"
-- Installing: /usr/local/lib64/libgraphblas.so.7.2.0
CMake Error at cmake_install.cmake:57 (file):
  file INSTALL cannot copy file
  "/gpfs/nvme1/1/casparl/ebbuildpath/SuiteSparse/5.13.0/foss-2022a-METIS-5.1.0/SuiteSparse-5.13.0/GraphBLAS/build/libgraphblas.so.7.2.0"
  to "/usr/local/lib64/libgraphblas.so.7.2.0": Permission denied.

Anyway, it's surely not the problem of this PR :) It works on generoso, that's good. Any chance you can upload a test report yourself? I always prefer to have at least two, if possible...

@Flamefire
Copy link
Contributor Author

Seems extremely weird no? Why would it be installing into my system prefix?

Indeed. I can't see why either and it works at our site.

( cd build ; make install )
...
Install the project...
-- Install configuration: "Release"
-- Installing: /usr/local/lib64/libgraphblas.so.7.2.0

That looks different (i.e. correct) in a test rebuild I just did (which succeeded)

Anyway, it's surely not the problem of this PR :) It works on generoso, that's good. Any chance you can upload a test report yourself? I always prefer to have at least two, if possible...

I was working on a Patch for that PyTorch Gloo issue and deleted my PyTorch installation. Rebuilding it and uploading a report after

@Flamefire Flamefire mentioned this pull request Mar 21, 2024
Merged
@Flamefire
Copy link
Contributor Author

Test report by @Flamefire
FAILED
Build succeeded for 2 out of 3 (3 easyconfigs in total)
n1227 - Linux RHEL 8.7 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (icelake), Python 3.8.13
See https://gist.github.com/Flamefire/1b8c09ac9e339a35b5ebeb67b5dfddf9 for a full test report.

@Flamefire
Copy link
Contributor Author

Test report above, forgot to exclude ReaxFF though.

I just double checked the SuiteSparse:

  • The lib is in GraphBLAS/build/libgraphblas.so.7.2.0 (relative to source/build root)
  • There is GraphBLAS/build/CMakeCache.txt (another in Mongoose/build but this is the relevant one)

indeed I have CMAKE_INSTALL_PREFIX:PATH= set correctly in that file (--disable-cleanup-build for eb to keep them)

Important seems to be make install INSTALL=<our prefix> being issued

casparvl
casparvl approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Ok, I'm going to approve this. It seems to work on at least 2 systems, the other issues are unrelated to this PR. I'll have to force the merge as we don't have the ReaxFF sources, and thus those tests failed.

@casparvl casparvl added this to the release after 4.9.0 milestone Mar 21, 2024
@casparvl
Copy link
Contributor

Going in, thanks @Flamefire!

@casparvl casparvl merged commit 5d25fed into easybuilders:develop Mar 21, 2024
9 checks passed
@Flamefire Flamefire deleted the w605_fix branch March 21, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casparvl casparvl casparvl approved these changes

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
4.9.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Flamefire @casparvl @boegelbot