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

Customize shebang 739 #940

Merged
merged 24 commits into from
Dec 21, 2017
Merged

Conversation

lekah
Copy link
Contributor

@lekah lekah commented Nov 20, 2017

This pull request resolves #739 by enforcing the customization of the shebang line for each computer.
For existing computers, the previous default is taken (#!/bin/bash)

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

minor fix

try:
return self._metadata['shebang']
except KeyError:
raise ConfigurationError('No workdir found for DbComputer {} '.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to change the exception message

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

A few additional comments:

  • we need to update the docs, telling what the shebang is, and what the default is
  • also in the docs we should say that the only supported ones are currently #!/bin/bash and #!/bin/bash -l; other shells might not work (e.g. for the way we escape commands, the way we set environment variables etc.)
  • once we merge we need to update also external repos relying on the number of inputs (there are a few docker files around, like aiida_docker_base, aiida_docker_compose, the ones in materials cloud, in the Quantum Mobile VM, ...)
  • I don't know if @DropD has been already working to a new verdi computer setup command with cmdline options, this has also to be updated. Probably at least these should be merged in develop so the external packages can use the command line options instead of a text file, and new (optional) options will not break external dependencies.


def set_shebang(self, val):
metadata = self._get_metadata()
# TODO: What are good checks?
Copy link
Member

Choose a reason for hiding this comment

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

A good check is that the first two characters are "#!" (they have to be); this would also implicitly check it's a string

#else:
# raise ModificationNotAllowed("Cannot set a property after having stored the entry")

def get_shebang(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can't this logic be put in the abstract class? It's a pity that it's copy-pasted, and also even more dangerously the default is duplicated. I think the function can just stay in the base class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think it should. The fact that there is a member called dbcomputer is a choice of the implementation. Some other backend could call it _dbcomputer or whatever, it's not enforced. As well as the function get_shebang, also not enforced. I would leave it where it is.

# This happens the first time: I provide a reasonable default value
return "#!/bin/bash"

def set_shebang(self, val):
Copy link
Member

Choose a reason for hiding this comment

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

Probably similarly for the other functions that are independent of the implementation, I would use the occasion to move them in the base class.


# I fill the list with the lines, and finally join them and return
script_lines = []
script_lines.append(shebang)
script_lines.append(job_tmpl.shebang)
Copy link
Member

Choose a reason for hiding this comment

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

Can we put here a default? Something like: if job_tmpl.shebang is None: ... else: ...? I see you changed all plugins, which is good, but I think there are a few that are developed as external plugins and might not be aware of this change.

Copy link
Member

Choose a reason for hiding this comment

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

(maybe verifying that the default is None, and leaving the option for an empty string to be dealt differently, if a user does not want the shebang line at all? I don't know if there is ever this need, but one never knows). Something like elif job_tmpl == "": #dont print anything, not even an empty line

@@ -908,6 +908,7 @@ def test_submit_script(self):
s = PbsproScheduler()

job_tmpl = JobTemplate()
job_tmpl.shebang = '#!/bin/bash -l'
Copy link
Member

Choose a reason for hiding this comment

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

You put a -l as a way to verify the current implementation?
Maybe if you implement also the case with shebang not specified, you can have a copy of one of these tests to verify that even if you don't set it, you get the default value?

if not val.startswith('!#'):
raise ValueError("A shebang line has to start with #!")
raise ValueError("{} is invalid. A shebang line has to start with #!".format(val))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this was intentional irony, but you actually swapped the # and ! around in the conditional 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think that if this is fixed, we still need @sphuber to accept the changes before merging?

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

I think you fixed Sebastiaan's comment, so it's good to merge for me

@sphuber sphuber merged commit da5a653 into aiidateam:develop Dec 21, 2017
@lekah lekah deleted the customize_shebang_739 branch December 22, 2017 14:59
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.

The shebang line at the beginning of the job script should be customizable
3 participants