vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
1.01k stars 120 forks source link

Append pid to spectator filename, if form -M #284

Open jodavies opened 6 years ago

jodavies commented 6 years ago

Make spectators nicer to use when running multiple concurrent jobs. We don't have to guarantee unique temp directories with the -M option.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 48.512% when pulling 3c2a74980e362807bb6c06d11185558cd50f7e7a on jodavies:spectatorsM into e8c97981ac4f7ac6d80006a2dff562c951f8c7ab on vermaseren:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.02%) to 48.512% when pulling 3c2a74980e362807bb6c06d11185558cd50f7e7a on jodavies:spectatorsM into e8c97981ac4f7ac6d80006a2dff562c951f8c7ab on vermaseren:master.

tueda commented 6 years ago

Indeed this is nice, though you will lose the perfect control of your spectator file names, for example, the file suffix (maybe it does matter, maybe not). The standard(?) solution is putting PID to the spectator file names, e.g., https://github.com/benruijl/forcer/blob/68a30bfb2aec3672a2f674ca8666bfc970105e24/forcer/forcer-manual.h#L131. I'm not sure if ((int)GetPID())%100000 could be always unique on a machine.

jodavies commented 6 years ago

Ah, I did not realize that the pid is available inside form. This also suffices. I think in principle ((int)GetPID())%100000 is not guaranteed to be uniqe, but it is what is used for form's other scratch files so I used it here also.

vermaseren commented 6 years ago

Hi Josh,

If you look in the file startup.c in the routine ReserveTempFiles and then look for the variable AM.MultiRun you will see how the -M flag is applied to the tmp files.

Cheers

Jos

On 20 Jun 2018, at 08:24, jodavies notifications@github.com wrote:

Ah, I did not realize that the pid is available inside form. This also suffices. I think in principle ((int)GetPID())%100000 is not guaranteed to be uniqe, but it is what is used for form's other scratch files so I used it here also.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/pull/284#issuecomment-398637898, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEnEtcNdz8hQkemL_NmvdEYJFG5Vbks5t-eqmgaJpZM4UtbZu.

jodavies commented 6 years ago

Yes, indeed, this is where I found that ((int)GetPID())%100000 was used for this mode. I simply copied the convention also for spectator files.

By the way, is there any reason the user should have complete control over the name of spectator files, in contrast to the name of all other temp files?

vermaseren commented 6 years ago

The reason is that you could have several spectator files at the same time. In that case you have to know how to address them. Imagine that I were to deal with the bebe and cross topologies in forcer simultaneaously. Either I have to put the ‘fallout’ into the same spaectator, or each in their own (as is done now, although we do not do it simultaneously). I just wanted to leave this possibility of having more than one.

Jos

On 20 Jun 2018, at 11:14, jodavies notifications@github.com wrote:

Yes, indeed, this is where I found that ((int)GetPID())%100000 was used for this mode. I simply copied the convention also for spectator files.

By the way, is there any reason the user should have complete control over the name of spectator files, in contrast to the name of all other temp files?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/pull/284#issuecomment-398681476, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEp73v17IsTznGsoT8IeD8nY0oW7aks5t-hKQgaJpZM4UtbZu.

tueda commented 6 years ago

I think this is not the reason why the user needs to have the full control of "real" file names. What the user needs is to have an unique name for each spectator expression, not for actual spectator file. In principle FORM could map logical spectator names into real unique file names.

jodavies commented 4 months ago

Aside from the user not being able to control the file extension (if that is really important I suppose the PID could be inserted before the extension?) is there any reason not to merge this one?

Given Issue #112 , I would actually think it is not a bad idea to just force -M mode in all cases. Then less can go wrong for users who are not aware of potential pitfalls.

tueda commented 4 months ago

Aside from the user not being able to control the file extension (if that is really important I suppose the PID could be inserted before the extension?) is there any reason not to merge this one?

I feel it's OK, but not enough to push it forward for me.

I would prefer to keep file extensions, but it needs some work: what if the case like /tmp/abc.123/spectator or on Windows.

In the first place, the root of the problem is why CreateSpectator needs the filename, I think. CreateSpectator <ExprName> should be enough at the user level, and then FORM could automatically assign the next spectator file name on the file system.

Any input from others?

Given Issue #112 , I would actually think it is not a bad idea to just force -M mode in all cases. Then less can go wrong for users who are not aware of potential pitfalls.

Maybe we need another issue focusing on the -M mode.

jodavies commented 4 months ago

Right, giving FORM full control over the filenames, such that it can take its "usual precautions" also solves this issue.

tueda commented 4 months ago

If any member with writing privileges to the repository wants to merge this PR, then I won't oppose it. It is true that the PR is one of the practical solutions that can be implemented easily and simply.