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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jodavies
Copy link
Collaborator

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

@coveralls
Copy link

Coverage Status

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

1 similar comment
@coveralls
Copy link

coveralls commented Jun 19, 2018

Coverage Status

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

@tueda
Copy link
Collaborator

tueda commented Jun 20, 2018

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
Copy link
Collaborator Author

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
Copy link
Owner

vermaseren commented Jun 20, 2018 via email

@jodavies
Copy link
Collaborator Author

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
Copy link
Owner

vermaseren commented Jun 20, 2018 via email

@tueda
Copy link
Collaborator

tueda commented Jun 20, 2018

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
Copy link
Collaborator Author

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
Copy link
Collaborator

tueda commented Feb 26, 2024

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
Copy link
Collaborator Author

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

@tueda
Copy link
Collaborator

tueda commented Feb 26, 2024

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.

@jodavies
Copy link
Collaborator Author

Any further thoughts on this one? I think relying on the user to put the PID in the spectator filename themselves in the form script when necessary is a recipe for problems in the end.

FORM5 could be the time to take user control of the spectator filenames away completely.

@tueda
Copy link
Collaborator

tueda commented Nov 27, 2024

Maybe no one will complain about this change to append PID to the names of spectator files.

But, a related question: can checkpoints still be recovered when sort files and/or spectator files have the PID appended?

Edit: sort files are not needed to recover, if I understand correctly.

@jodavies
Copy link
Collaborator Author

Good question. The sort files can always be called various names depending on whether form sees an "xxx" file already. I will try to test -- I have never used the checkpoint mechanism before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants