-
Notifications
You must be signed in to change notification settings - Fork 9
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
DistRDF fixes #41
base: main
Are you sure you want to change the base?
DistRDF fixes #41
Conversation
It doesn't work.
'm not 100% sure it's needed, but it should not hurt.
- wrap CompileMacro call to make it thread-safe and check its return value - add several sanity checks
06f5627
to
6c6cc54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! See some comments from my side
cpp_source = "helpers.h" | ||
|
||
ROOT.gSystem.CompileMacro(str(cpp_source), "kO") | ||
print("Not on a worker") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print("Not on a worker") |
Not really needed, more for debugging purposes
print("Compiling the macro.") | ||
library_source = "helpers.h" | ||
local_dir = get_worker().local_directory | ||
library_path = os.path.join(local_dir, library_source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also check that library_path
points to an existing file, to catch file not found problems earlier?
load_cpp() | ||
ml.load_cpp(fastforest_path) | ||
|
||
# TODO: make ml.load_cpp working on distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not working after this patch?
def load_all(fastforest_path): | ||
load_cpp() | ||
ml.load_cpp(fastforest_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably deserves a comment as to why it's needed.
@@ -74,6 +76,24 @@ class MLHistoConf: | |||
] | |||
|
|||
|
|||
def compile_macro_wrapper(library_path: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function is copied verbatim from the analysis, should it live in some utilities module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, probably deserves a bit more explanation in the first cell, i.e. that this is a notebook to show how to potentially run the analysis on SWAN+HTCondor. Then there's the usual problem of keeping the notebook up to date with the Python script, but I don't know if that fits in that PR
Here are some fixes that I applied in order to get to a working configuration with DistRDF and HTCondor on SWAN. This brings the code in a neighborhood of a working configuration.
I have seen i hang indefinitely on SWAN sometimes, but I'm not sure whether the problem was on the application or the infrastructure side.