Closed HeloiseS closed 2 years ago
jeffriley
commented
2 years ago Thanks Heloise! The funny thing is I noticed the typo in the C++ code - and fixed it - but I didn't think to look in pythonSubmit. My guess is the author of that new part of the code copy-and-pasted from the C++ code before I noticed the typo and fixed it.
We'll push a fix.
@ilyamandel fyi. We also should get the documentation for the new option online.
HeloiseS
commented
2 years ago Aces. What about the log file record error? What did I do wrong or miss out?
ilyamandel
commented
2 years ago Hi @HeloiseS,
surive is entirely my fault -- I made a typo in the latest version that I committed on Monday. Thanks, @jeffriley!
BTW, do you want to review the living code or a particular tag (we tagged v02.21.00, which is the reference version for the ApJS submission)?
jeffriley
commented
2 years ago Aces. What about the log file record error? What did I do wrong or miss out?
Sorry - forgot to get back t o that. I'll take a look later tonight or tomorrow.
HeloiseS
commented
2 years ago @ilyamandel that's interesting. How did you end up with 4 identical typos across 2 files? did you accidentally upload old versions? You don't really need to asnwer here, the reason I'm asking is that ideally one would want to avoid that sort of stuff from happening and if it does (because we're all human after all) to get caught before it gets on the release branch, which brings me to continuous integration and unitest: one of JOSS boxes we'll have to discuss. Unitesting a giant C++ star evolution code might not be practical, but having a system to avoid that sort of bug in the surrounding applications (particularly the ones used to run it) would be important.
As for what version I'm reviewing: the living code :) currently on compas 2.25 I believe.
jeffriley
commented
2 years ago How did you end up with 4 identical typos across 2 files?
That would be partly my fault :-) @ilya updated preProcessing/pythonSubmit.py, and presumably copy-pasted the incorrectly spelled option name from the C++ source. Later I noticed thetypo in the C++ source and fixe it, but didn't realise it had been copy-pasted to preProcessing/pythonSubmit.py. While making some mods to the pythonSubmit files in the examples directories, I thought I'd add the new options to those pyhtonSubmit files, so I copy-pasted the new (incorrectly spelled) options from preProcessing/pythonSubmit.py.
jeffriley
commented
2 years ago I couldn't figure out why it wasn't happy, the COMPAS_Output_Definitions.txt is in the directory where i got the pythonSubmit file
That's the problem. When COMPAS is run in a docker image, it picks up the environment variables COMPAS_LOGS_OUTPUT_DIR_PATH and COMPAS_INPUT_DIR_PATH.
$COMPAS_LOGS_OUTPUT_DIR_PATH points to the directory to which the log files are written (so for COMPAS output files) $COMPAS_INPUT_DIR_PATH points to the directory from which the grid file and logfile-definitions files are read (so for COMPAS input files)
pythonSubmit.py is not a COMPAS input file, so it (probably) won't be in $COMPAS_INPUT_DIR_PATH (so putting the logfile-definitions file in the same directory as pythonSubmit.py almost certainly means it (the logfile-definitions file) is in the wrong directory (or at least is in a directory COMPAS doesn't know to look in). The COMPAS executable knows nothing about pythonSubmit.py - all pythonSubmit.py does is construct the command line to execute COMPAS, and then executes it. COMPAS, now running, goes and looks in $COMPAS_INPUT_DIR_PATH for its input files (grid and log-file-definitions), and puts its output in $COMPAS_LOGS_OUTPUT_DIR_PATH
So the command you should run for docker is:
sudo docker run
--rm
-it
-v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs
-v
$COMPAS_ROOT_DIR/examples/methods_paper_plots/chirpmass_distribution/pythonSubmit.py:/app/starts/pythonSubmit.py
-e COMPAS_EXECUTABLE_PATH=/app/COMPAS/bin/COMPAS
-e COMPAS_INPUT_DIR_PATH=/app/COMPAS/config
-e COMPAS_LOGS_OUTPUT_DIR_PATH=/app/COMPAS/logs
teamcompas/compas
python3 /app/starts/pythonSubmit.py
(where /app/COMPAS/config is the directory COMPAS will look in to find the grid file and/or the logfile-definitions file)Not your fault of course - our example doesn't show it, because it doesn't use a grid file or a logfile-definitions file. I'll make a note to update the example.
Thanks for finding the omission :-)
HeloiseS
commented
2 years ago I still get the same error so I have some follow up questions:
goes and looks in $COMPAS_INPUT_DIR_PATH
Where is that? I went into the pythonSumbit file (I remembered seeing it in there) and there is a nicely detailed comment (see below), that I thought meant I was supposed to have the COMPAS_Output_Definitions.txt in the current working directory, but I still got the same error.
# environment variable COMPAS_INPUT_DIR_PATH is used primarily for docker runs
# if COMPAS_INPUT_DIR_PATH is set (!= None) it is prepended to input filenames
# (such as grid_filename and logfile_definitions)
# if COMPAS_INPUT_DIR_PATH is not set (== None) the current working directory
# is prepended to input filenamesSo let's get back to basics:
1) Where am i supposed to be? I assumed i could be anywhere on my computer given the environment variable points to compas, but maybe for the config files i need to be in a specific location?
2) What really is going on with COMPAS_INPUT_DIR_PATH and COMPAS_OUTPUT_DIR_PATH in this case? It looks like we are defining them in the command (and i actually removed the -e COMPAS_INPUT_DIR_PATH=/app/COMPAS/config at one point thinking, maybe if i didn't set it it would default to looking ind my cwd), but that looks like a location in the docker image not on my computer so I'm a tad confused.
I initially copied the output definitions to $COMPAS_ROOT_DIR/compas-logs (i don't know what i was trying to achieve :rofl:). It made me realise that running docker created a directory compas-log in my $COMPAS_ROOT_DIR that is locked (I am mainly a GUI user, and I see a little lock on the directory picture), the permisions were: drwxr-xr-x. I also realised that it wasn't empty!
I found a COMPAS_Output folder with the results from the docker run i did with the command line (btw I might as well tell you know, but I think the docs need updating because the command --number-of-binaries=5 gave me an error saying that it didn't exist, I replaced it with --number-of-systems and it ran :ok_hand:
Some Questions
Why are the permissions different on those directories than the ones created via not-docker? Not saying it needs changing just wondering, because I very aggressively sudo chmod 777-ed the directory - is that not recommended? I dunno if the permission setting is a fun and quirky docker thing that you're happy for users to update or if there's a reason for it and I shouldn't mess with it.
What's the time recorded in the log files? Is it different on docker runs and command line runs? The reason I ask is because in that log I have Start generating binaries at Wed Nov 3 05:00:29 2021 and based on other runs that time is in 24 hour format, but I certainly didn't run compas at 5 AM so that's weird?
ilyamandel
commented
2 years ago PR #681 fixes the surive->survive typos and updates the documentation to include the new introduced CE options. I leave the other questions for @jeffriley . :)
Aside: @jeffriley , do we still need the documentation in docs (rather than docs/online-docs)? For example, https://github.com/TeamCOMPAS/COMPAS/blob/dev/docs/getting_started.md is out of date [see, e.g., reference to non-existing .pdf file in the final paragraph] -- but rather than updating it (and always remembering that we have two versions to update), should we just delete it?
jeffriley
commented
2 years ago do we still need the documentation in docs (rather than docs/online-docs)? For example, https://github.com/TeamCOMPAS/COMPAS/blob/dev/docs/getting_started.md is out of date
I'd be happy to remove them, otherwise, as you say, we'd have to keep them in sync with the online documentation - they were created by @reinhold-willcox and @themikelau I think, so they might have an opinion.
themikelau
commented
2 years ago Happy to get rid of the documents in the docs directory as long as we have made sure we don't refer to them anywhere.
jeffriley
commented
2 years ago Where is COMPAS_Output_Definitions.txt meant to go? goes and looks in $COMPAS_INPUT_DIR_PATH Where is that?
There are two separate concepts here.
First, COMPAS needs to know where to look for its input files (grid file, logfile-definitions file), and it needs to know where to put its output (log) files.
The default for both of these is CWD.
COMPAS provides a program option (command-line argument) to specify the path to be used for output files: --ouput-path
./compas --mode BSE -n 1000 --output-path path-to-where-user-wants-logfilesThere is no program option to specify the directory in which the input files reside (maybe there should be - never thought about it until now), so if the user wants the input files in a specific directory, they need to use a fully-qualified filename when the specify the input files:
./compas --mode BSE -n 1000 --output-path path-to-where-user-wants-logfiles --grid fully-qualified-path-to-grid-file --logfile-definitions fully-qualified-path-to-logfile-definitions-fileSo that's how COMPAS manages input and output files.
pythonSubmit.py will use the following environment variables if they are set:
COMPAS_INPUT_DIR_PATH
COMPAS_LOGS_OUTPUT_DIR_PATHIf COMPAS_INPUT_DIR_PATH is set, pythonSubmit.py will prepend it to any input filenames specified (grid file, logfile-definitions file). If COMPAS_INPUT_DIR_PATH is NOT set, pythonSubmit.py will prepend the CWD to any input filenames specified (grid file, logfile-definitions file) - not strictly necessary, because the COMPAS executable will default to the CWD anyway.
If COMPAS_LOGS_OUTPUT_DIR_PATH is set, pythonSubmit.py use $COMPAS_LOGS_OUTPUT_DIR_PATH as the value for the '--output-path' program option. If COMPAS_LOGS_OUTPUT_DIR_PATH is NOT set, pythonSubmit.py will use the CWD as the value for the '--output-path' program option - not strictly necessary, because the COMPAS executable will default to the CWD anyway.
Second, COMPAS can be run on a physical machine, or in a VM using the docker image.
The user can't put the input files in a directory in the VM, nor can they access the output (log) files in a directory in the VM (directly, anyway). We have to map a directory on a physical machine to a directory in the VM. That's what the "-v" options to the 'docker run' command do.
-v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs maps the directory $COMPAS_ROOT_DIR/compas-logs on a physical system to the /app/COMPAS/logs directory in the VM.
Putting both of those together should help explain the 'docker run' options, and where the files should be.
jeffriley
commented
2 years ago Why are the permissions different on those directories than the ones created via not-docker
I hadn't noticed. It's probably something docker does (I haven't checked), but certainly after the docker run is finished you can freely change the permission to whatever suits you
What's the time recorded in the log files?
The time of the run in Run_Details? That's the time on the system used to run COMPAS. We set the timezone in our docker images to UTC/GMT, which would explain what you're seeing (NZDT is UTC+13 I think). We did that because our docker image can be run anywhere - I've done a number of runs on AWS with 32 nodes, and those nodes were in the US somewhere (may have even been across timezones), so recording local time wouldn't make sense.
ilyamandel
commented
2 years ago Since there are now multiple threads here, I opened up docs/ cleaning as a separate issue, #686 .
HeloiseS
commented
2 years ago Thanks for the detailed explenation @jeffriley now the docker command makes complete sense - it still doesn't work :laughing: but i can start asking questions more confidently:
If COMPAS_INPUT_DIR_PATH is NOT set, pythonSubmit.py will prepend the CWD to any input filenames specified
Okay that's what i thought, and I did make sure the COMPAS_Output_definitions.txt was in my CWD. I haven't set COMPAS_INPUT_DIR_PATH, I put nothing in my .bashrc, and i checked that doing$COMPAS_INPUT_DIR_PATH in my shell gives me nothing, so I think it's fair to assume it will default.
So why isn't it seeing the COMPAS_Output_definitions.txt that's in my cwd?
I don't know if it'll help but here is the directory i'm running things in
fste075@kvin:~/Documents/reviews/COMPAS$ ls -al
total 204
drwxr-xr-x 4 fste075 62215 4096 Nov 4 17:45 .
drwxr-xr-x 8 fste075 62215 4096 Oct 30 13:40 ..
-rw-r--r-- 1 fste075 62215 166138 Oct 30 13:56 10.21105.joss.03838.pdf
drwxr-xr-x 3 fste075 62215 4096 Nov 4 15:54 COMPAS_Output
drwxr-xr-x 2 fste075 62215 4096 Nov 3 16:51 COMPAS_Output_2
-rw-r--r-- 1 fste075 62215 1319 Oct 30 14:07 COMPAS_Output_Definitions.txt
-rw-r--r-- 1 fste075 62215 16526 Nov 4 17:45 review.log
but certainly after the docker run is finished you can freely change the permission to whatever suits you
The time of the run in Run_Details? That's the time on the system used to run COMPAS. We set the timezone in our docker images to UTC/GMT, which would explain what you're seeing (NZDT is UTC+13 I think). We did that because our docker image can be run anywhere - I've done a number of runs on AWS with 32 nodes, and those nodes were in the US somewhere (may have even been across timezones), so recording local time wouldn't make sense.
jeffriley
commented
2 years ago So why isn't it seeing the COMPAS_Output_definitions.txt that's in my cwd?
Are you running on your physical system, or using a docker image? Can you share the command you are using to run?
HeloiseS
commented
2 years ago Docker: I'm using the command you gave me the other day :)
sudo docker run --rm -it -v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs -v $COMPAS_ROOT_DIR/examples/methods_paper_plots/chirpmass_distribution/pythonSubmit.py:/app/starts/pythonSubmit.py -e COMPAS_EXECUTABLE_PATH=/app/COMPAS/bin/COMPAS -e COMPAS_INPUT_DIR_PATH=/app/COMPAS/config -e COMPAS_This is my fault - sorry.
Your command is this:
sudo docker run
--rm
-it
-v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs
-v
$COMPAS_ROOT_DIR/examples/methods_paper_plots/chirpmass_distribution/pythonSubmit.py:/app/starts/pythonSubmit.py
-e COMPAS_EXECUTABLE_PATH=/app/COMPAS/bin/COMPAS
-e COMPAS_INPUT_DIR_PATH=/app/COMPAS/config
-e COMPAS_LOGS_OUTPUT_DIR_PATH=/app/COMPAS/logs
teamcompas/compas
python3 /app/starts/pythonSubmit.pyThe command-line argument:
-v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs maps the VM directory /app/COMPAS/logs to the directory $COMPAS_ROOT_DIR/compas-logs on your physical system. And the argument:
-e COMPAS_LOGS_OUTPUT_DIR_PATH=/app/COMPAS/logssets the environment variable COMPAS_LOGS_OUTPUT_DIR_PATH in the VM to /app/COMPAS/logs, so COMPAS will write its log files to /app/COMPAS/logs in the VM, which is mapped to $COMPAS_ROOT_DIR/compas-logs, so the COMPAS logs will be in the directory $COMPAS_ROOT_DIR/compas-logs on your physical system
This command-line argument:
-e COMPAS_INPUT_DIR_PATH=/app/COMPAS/configsets the environment variable COMPAS_INPUT_DIR_PATH in the VM to /app/COMPAS/config, so COMPAS (via pythonSubmit.py) will look in /app/COMPAS/config in the VM for any grid file or logfile-definitions file, but we haven't mapped that directory anywhere!
Sorry, this is my fault - I didn't look carefully enough when I added the command-line argument:
-e COMPAS_INPUT_DIR_PATH=/app/COMPAS/configI should have added this mapping (I assumed it was there - I don't know why):
-v directory-on-physical-system-for-input-files:/app/COMPAS/configwhere "directory-on-physical-system-for-input-files" should be (in this case) "~/Documents/reviews/COMPAS" (or the path to the directory on t he physical system in which the logfile-definitions file resides)
So the "input" directory and the "output" directory both need the respective environment variable set, and a mapping physical->virtual.
Sorry - I should have paid closer attention when I answered the question initially.
HeloiseS
commented
2 years ago lmao :rofl: I'm going insane it's still not working.
more the file with $COMPAS_INPUT_DIR_PATH/COMPAS_Output_Definitions.txt.=> the error is File does not exist: /app/COMPAS/COMPAS_Output_Definitions.txt so from what i understand it's not even looking in /app/COMPAS/config it's looking in /app/COMPAS/... so is there a problem in the pythonSubmit where it's not making the command properly to look in the config directory?
It would make sense if the python file was the problem actually because i ran docker no problem with the command line version
jeffriley
commented
2 years ago so is there a problem in the pythonSubmit where it's not making the command properly to look in the config directory?
It would seem so...
So, the version of the pythonSubmit.py file I'm looking at has:
compas_input_path_override = os.environ.get('COMPAS_INPUT_DIR_PATH')
...
if logfile_definitions != None:
# if the grid filename supplied is already fully-qualified, leave it as is
head, tail = ntpath.split(logfile_definitions) # split into pathname and base filename
if head == '' or head == '.': # no path (or CWD) - add path as required
logfile_definitions = tail or ntpath.basename(head)
if compas_input_path_override == None:
logfile_definitions = os.getcwd() + '/' + logfile_definitions.strip("'\"")
else:
logfile_definitions = compas_input_path_override + '/' + logfile_definitions.strip("'\"")Leaving aside the typo (# if the grid filename supplied), that should prepend whatever is in the environment variable COMPAS_INPUT_DIR_PATH to the logfile-definitions file name.
Oh wait... that will fail if the logfile-definitions file already has a path prepended to it. Hmmm. Might have to rethink that code. So, if the logfile-definitions file (and the grid file if you use one), is not just a bare filename, then you should make it one (i.e. no path prepended), and use the environment variable to prepend the path.
So what does the logfile-definitions filename look like in your pythonSubmit file? (Also grid file name)
HeloiseS
commented
2 years ago SORRY FOR THE SPAM - realised i misread something so just reposting the correct information
TL;DR: logfile_definitions is okay I think and pythonSubmit broken in 2.25?
[----see below----]
I tried running the pythonSubmit file alone (it's the one in chirpmassDistributions btw) to try and print you the logfile_definitions but :musical_note: it faiiiiled :musical_note: did y'all update the commands again? I hadn't rerun the files since 2.24 but it seems there's extra stuff in v2.25
Commandline Options error: unrecognised option '--common-envelope-allow-radiative-envelope-survive'
This command doens't exist in your user manual, so (without looking at the PR) I'm assuming it came form the recent update - both my pythonSubmit and pythonSubmitDemo have it and don't run.
I tried removing the problematic command and there is another one that's causing me grief --common-envelope-allow-immediate-rlof-post-ce-survive. I rebuilt the c++ because altho I pulled the new PR I hadn't run the makefile again... stil broken. When i git pull, does it know if the c++ has changed? I'm wondering if maybe it jsut didn't get the updated src
HeloiseS
commented
2 years ago My bad I copied stuff form the wrong pythonSubmit. Above was pythonSubmitDemo, the pythonSubmit in chirpmassDistribution DOES have logfile_definitions = "COMPAS_Output_Definitions.txt" and therefore the logfile_definitions come out as i expected. Huzza!
Still docker doesn't work hahah.
here is the code you asked for from the correct file this time
grid_filename = None # grid file name (e.g. 'mygrid.txt')
if grid_filename != None:
# if the grid filename supplied is already fully-qualified, leave it as is
head, tail = ntpath.split(grid_filename) # split into pathname and base filename
if head == '' or head == '.': # no path (or CWD) - add path as required
grid_filename = tail or ntpath.basename(head)
if compas_input_path_override == None:
grid_filename = os.getcwd() + '/' + grid_filename.strip("'\"")
else:
grid_filename = compas_input_path_override + '/' + grid_filename.strip("'\"")
logfile_definitions = "COMPAS_Output_Definitions.txt" # logfile record definitions file name (e.g. 'logdefs.txt')
if logfile_definitions != None:
# if the grid filename supplied is already fully-qualified, leave it as is
head, tail = ntpath.split(logfile_definitions) # split into pathname and base filename
if head == '' or head == '.': # no path (or CWD) - add path as required
logfile_definitions = tail or ntpath.basename(head)
if compas_input_path_override == None:
logfile_definitions = os.getcwd() + '/' + logfile_definitions.strip("'\"")
print('blaaa')
else:
logfile_definitions = compas_input_path_override + '/' + logfile_definitions.strip("'\"")--common-envelope-allow-radiative-envelope-survive and --common-envelope-allow-immediate-rlof-post-ce-survive are in the online documentation:
You may need to reload the page - you could still have the old version in your browser cache (we updated it a couple of days ago).
Ok, the pythonSubmit looks ok. Does your docker command include the mapping? I know you have already put the docker command here, but can you do it again (because you should have added to it since the last time). Then I'll have everything I need to try to reproduce what you're seeing.
HeloiseS
commented
2 years ago You may need to reload the page - you could still have the old version in your browser cache (we updated it a couple of days ago).
you're right :)
Still why does it error? Do I need to manually redownload the c++ code before I remake it?
I know you have already put the docker command here, but can you do it again (because you should have added to it since the last time).
No problem here it is.
sudo docker run --rm -it -v $COMPAS_ROOT_DIR/compas-logs:/app/COMPAS/logs -v $COMPAS_ROOT_DIR/examples/methods_paper_plots/chirpmass_distribution/pythonSubmit.py:/app/starts/pythonSubmit.py -e COMPAS_EXECUTABLE_PATH=/app/COMPAS/bin/COMPAS -e COMPAS_INPUT_DIR_PATH=/app/COMPAS/config -v ~/Documents/reviews/COMPAS=/app/COMPAS/config -e COMPAS_LOGS_OUTPUT_DIR_PATH=/app/COMPAS/logs teamcompas/compas python3 /app/starts/pythonSubmit.pyStill why does it error? Do I need to manually redownload the c++ code before I remake it?
You need to do something like 'git pull upstream dev' (depending upon how you've set things up. 'git remote -v' will tell you what upstream and origin etc are pointing to).
I'll look at the docker problem later. Do you get the same error?
HeloiseS
commented
2 years ago facepalm it's not missing.... I typed a = instead of the : in -v ~/Documents/reviews/COMPAS=/app/COMPAS/config
I'm so sorry!!!!
anyways it worked!!
okay i have completely lost track of what needs updating in the manual - I trust you haven't? What I can do is that when the review is pretty much complete i can delete compas off my computer and then rerun everything clean just to double check.
But for now it's done! woohoo thanks a bunch :)
You need to do something like 'git pull upstream dev' (depending upon how you've set things up. 'git remote -v' will tell you what upstream and origin etc are pointing to).
I'll try to fix that later.
jeffriley
commented
2 years ago anyways it worked!!
Great!
okay i have completely lost track of what needs updating in the manual - I trust you haven't?
Yeah, I think I know what needs updating!
ilyamandel
commented
2 years ago @jeffriley -- are there specific instructions that still need updating from this issue? If not, can we close the issue?
ilyamandel
commented
2 years ago @jeffriley -- see my question above; this issue ended up in a sufficiently long discussion that I can't figure out if there's actually a TODO item here. :)
jeffriley
commented
2 years ago @ilyamandel I'll look through this and close/take action as appropriate
jeffriley
commented
2 years ago Documentation changes for docker issues in PR #724
@ilyamandel We should address the multiple pythonsubmit files - but maybe that can be address as part of the pythonsubmit refactor (PR #720)?
This issue is submitted as part of the JOSS review #3838
:bug: description
The typo
so I was testing running compas in a docker image (it all installed beautifully, very easy) and i got the following error [Abridged]
Commandline Options error: unrecognised option '--common-envelope-allow-radiative-envelope-surive'I used one of your original pythonSubmit files to run this (the one in examples/*/chirpmass_distribution), and when i go look in the file it does seem that the commands and the python variables associated with
--common-envelope-allow-radiative-envelope-surviveall have that typo in them (surive instead of survive). Same for the pythonSubmitDemo from looking at the github.The log file
After correcting the typo in the command I ran it again and got the following [Abridged]
I couldn't figure out why it wasn't happy, the COMPAS_Output_Definitions.txt is in the directory where i got the pythonSubmit file :shrug: - I'll need your help to fix this one :)
The Command I ran
Full outputs
Full Output Bug 1
Full Output bug 2