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

Copy build log and artifacts to a permanent location after failures #4601

Open
wants to merge 7 commits into
base: 5.0.x
Choose a base branch
from

Conversation

gkaf89
Copy link

@gkaf89 gkaf89 commented Aug 5, 2024

The files can be build in some selected build path (--buildpath), and the logs of successful compilation are then concentrated to some other location for permanent storage (--logfile-format). Logs of failed builds remain in the build path location so that they can be inspected.

However, this setup is problematic when building software in HPC jobs. Quite often in HPC systems the build path is set to some fast storage local to the node, like NVME raid mounted on /tmp or /dev/shm (as suggested in the documentation: https://docs.easybuild.io/configuration/#buildpath). The node storage is often wiped out after the end of a job, so the log files and the artifacts are no longer available after the termination of the job.

This commit adds an option to accumulate errors in some more permanent location, so they can be easily inspected after a failed build.

@gkaf89 gkaf89 marked this pull request as draft August 5, 2024 13:10
@gkaf89 gkaf89 force-pushed the feature/error-logging branch 2 times, most recently from b306ccd to 092fcd0 Compare August 5, 2024 13:26
@gkaf89
Copy link
Author

gkaf89 commented Aug 5, 2024

I am not sure what is the best way to select the build directory so that I can move it to a more permanent location. That is at the moment I am recreating the location of the build path and then copy the directory to the destination path:

source_build_path = os.path.join(buildpath, name, version, toolchain)
dest_build_path = os.path.join(err_log_path, name, version, toolchain)
copy_dir(source_build_path, dest_build_path)
  • Is there some variable holding the build path, or even the relative build path (i.e. os.path.join(name, version, toolchain))?
  • Should we extract this functionality to a module?

@gkaf89 gkaf89 force-pushed the feature/error-logging branch 7 times, most recently from 50d99c3 to 86fe081 Compare August 12, 2024 19:10
@boegel boegel added this to the 4.x milestone Aug 13, 2024
@boegel
Copy link
Member

boegel commented Aug 14, 2024

@gkaf89 The builddir variable that is set in each easyblock instance hold the path to the build directory for that particular easyconfig.
You can determine the relative path via the build_path() function that is available from easybuild.tools.config, that should report the top directory that corresponds to the buildpath EasyBuild configuration option (see also https://docs.easybuild.io/configuration/#buildpath).

For, for example, for example-1.2.3-GCC-12.3.0.eb, the builddir path would be something like /tmp/myuser/easybuild/build/example/1.2.3/GCC-12.3.0/, with buildpath set to /tmp/myuser/easybuild/build.
Not that the actual build directory in which the compilation is being done would be one level deeper, corresponding to the unpacked source tarball, so something like /tmp/myuser/easybuild/build/example/1.2.3/GCC-12.3.0/example-1.2.3/.

So, I think you could create a subdirectory in the permanent storage location that uses the name of the easyconfig file (to keep it simple), and copy the contents of builddir in there.
You do somehow want to make sure that the target path is unique though, because you could have multiple builds ongoing on different nodes that would all copy to the same permanent location in the end...

@akesandgren
Copy link
Contributor

@gkaf89 The builddir variable that is set in each easyblock instance hold the path to the build directory for that particular easyconfig. You can determine the relative path via the build_path() function that is available from easybuild.tools.config, that should report the top directory that corresponds to the buildpath EasyBuild configuration option (see also https://docs.easybuild.io/configuration/#buildpath).

For, for example, for example-1.2.3-GCC-12.3.0.eb, the builddir path would be something like /tmp/myuser/easybuild/build/example/1.2.3/GCC-12.3.0/, with buildpath set to /tmp/myuser/easybuild/build. Not that the actual build directory in which the compilation is being done would be one level deeper, corresponding to the unpacked source tarball, so something like /tmp/myuser/easybuild/build/example/1.2.3/GCC-12.3.0/example-1.2.3/.

So, I think you could create a subdirectory in the permanent storage location that uses the name of the easyconfig file (to keep it simple), and copy the contents of builddir in there. You do somehow want to make sure that the target path is unique though, because you could have multiple builds ongoing on different nodes that would all copy to the same permanent location in the end...

Yeah, the thing to copy should be builddir into a path with the diff of buildpath and builddir based in permanent-storage-location. Just make sure to remove old remnants of that first :-)

@gkaf89 gkaf89 force-pushed the feature/error-logging branch 4 times, most recently from 1274a2b to b1a9da8 Compare August 23, 2024 08:53
@boegel
Copy link
Member

boegel commented Aug 27, 2024

@gkaf89 If you need any help with this, do let us know!

@gkaf89 gkaf89 force-pushed the feature/error-logging branch from b1a9da8 to 6bc53e6 Compare September 8, 2024 22:43
@gkaf89
Copy link
Author

gkaf89 commented Sep 8, 2024

@boegel The commit is ready. I won't have enough time to familiarize myself with the test framework for the EasyBlockTest class to prepare a test before the next release.

The commit can be tested by modifying the configuration options of some easyconfig that uses the system toolchain to cause a failure. For instance I added the option

configopts = '--some-invalid-option'

in zlib-1.3.1.eb. The result is that the temporary log file in the build directory and the extracted source code are copied in a permanent location.

@gkaf89 gkaf89 marked this pull request as ready for review September 8, 2024 23:38
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
@boegel
Copy link
Member

boegel commented Sep 11, 2024

@gkaf89 There's a problem with the tests, looks like test_toy_build was broken by the changes being made here?
See for example https://github.com/easybuilders/easybuild-framework/actions/runs/10805964835/job/29973948603

@gkaf89
Copy link
Author

gkaf89 commented Sep 11, 2024

The failure is caused because the target location for permanent storage is the same as the source location. The steps I am following to resolve the issue:

  • add a source/destination check to avoid a hard failure, and
  • detect how the source and the destination path end with the same value in the test.

@gkaf89 gkaf89 force-pushed the feature/error-logging branch from b34ff11 to 5cfdfd1 Compare September 11, 2024 10:24
@gkaf89
Copy link
Author

gkaf89 commented Sep 11, 2024

@boegel Some edge cases where uncovered by the tests. The latest commit resolves the issue.

I leave it up you if you prefer to move it to version 5. I am not familiar enough with the tests to test the PR extensively.

@gkaf89 gkaf89 requested a review from boegel September 11, 2024 12:51
@boegel
Copy link
Member

boegel commented Dec 4, 2024

@gkaf89 As briefly discussed during the conf call today, it would be good if you could add a test (or enhance an existing one, like test_toy_broken) to verify that the added functionality works as intended (and keeps working).

Do let us know if you need any help with that!

@gkaf89 gkaf89 force-pushed the feature/error-logging branch 2 times, most recently from e0d3a6e to 222041d Compare January 27, 2025 08:25
@gkaf89 gkaf89 requested a review from Flamefire January 27, 2025 08:32
@gkaf89 gkaf89 force-pushed the feature/error-logging branch from 222041d to c2035a5 Compare January 28, 2025 16:53
easybuild/base/testing.py Outdated Show resolved Hide resolved
@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Jan 29, 2025
@boegel boegel modified the milestones: 4.x, 5.0 Jan 29, 2025
@gkaf89 gkaf89 force-pushed the feature/error-logging branch from c2035a5 to bf40ad9 Compare January 30, 2025 10:13
@gkaf89 gkaf89 requested a review from boegel January 30, 2025 10:18
@gkaf89 gkaf89 force-pushed the feature/error-logging branch from bf40ad9 to 04f4c50 Compare January 31, 2025 11:44
Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Just did a quick scan as I think everything has been discussed already. Just 2 minor improvements

test/framework/filetools.py Outdated Show resolved Hide resolved
test/framework/toy_build.py Outdated Show resolved Hide resolved
The create_unused_dir function distinguishes between the parent
directory and the name of the target subdirectory in 2 separate
arguments. This interface requires additional parameter management when
combined with newer path management interfaces such as pathlib.

- Add a function create_unused_dirs accepting the target paths as a
  single argument.
- Support the simultaneous creation of multiple unused paths with all of
  them having the same suffix if any of the actually requested paths is
  unavailable.
Packages are be built in some selected build path (--buildpath), and
the logs of successful compilation are then concentrated to some other
location for permanent storage (--logfile-format). Logs of failed builds
remain in the build path location so that they can be inspected.

However, this setup is problematic when building software in HPC jobs. Quite
often in HPC systems the build path is set to some fast storage local to
the node, like NVME raid mounted on `/tmp` or `/dev/shm` (as suggested
in the documentation: https://docs.easybuild.io/configuration/#buildpath).
The node storage is often wiped out after the end of a job, so the log
files and the artifacts are no longer available after the termination of
the job.

This commit adds options (--log-error-path and --artifact-error-path) to
accumulate error logs and artifacts in some more permanent locations, so
that the can be easily inspected after a failed build. By default,
neither the logs nor the build artifacts are copied in the permanent
location.
Flags for persisting logs and artifacts are now checked during the
validation of input options. In case of invalid settings the program
terminates early with an error.

Integration tests for internal logging functions are deactivated as the
logging functions are never reached in normal operation.
- Avoid single character variables names outside list comprehension.
- Use a consistent pattern in function calls throughout the pull request.
- Use context manager to mock stdout and stderr.
@gkaf89 gkaf89 force-pushed the feature/error-logging branch from 04f4c50 to b1e815b Compare February 7, 2025 13:37
@gkaf89
Copy link
Author

gkaf89 commented Feb 7, 2025

After using the commit for a while, I made a minor interface change. Now the report message mentions the actual path to the artifacts, which speeds up the navigation to the artifact location.

@gkaf89 gkaf89 requested a review from Flamefire February 7, 2025 13:40
@Flamefire
Copy link
Contributor

After using the commit for a while, I made a minor interface change. Now the report message mentions the actual path to the artifacts, which speeds up the navigation to the artifact location.

Please don't force-push in the middle of a review as I can't tell for sure what has changed. Is only the last commit new or did any of the others change as well?

@gkaf89 gkaf89 force-pushed the feature/error-logging branch 4 times, most recently from c9e9963 to c0cde17 Compare February 8, 2025 09:38
When copying logs and artifacts from failed builds, mention the full
target path instead of its parent. This reduces the number of actions
required to access the artifacts (select, paste, and enter).
@gkaf89 gkaf89 force-pushed the feature/error-logging branch from c0cde17 to ea723dd Compare February 8, 2025 09:59
@gkaf89
Copy link
Author

gkaf89 commented Feb 8, 2025

@Flamefire Sorry, I try to keep the commit history organized. I will try to thing of a way to limit forced pushes next time.

All changes start from the [STYLE] commit, older commits were not modified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

4 participants