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

Uw 44 generate generic scheduler tool #19

Closed
wants to merge 11 commits into from

Conversation

ope-adejumo
Copy link
Contributor

Description

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

@christinaholtNOAA
Copy link
Contributor

@ope-adejumo Thanks for addressing some of the concerns we'd discussed in person here! It's really shaping up. Just because our future selves may thank us for being more present in the PR conversation, I am going to attempt to summarize a few offline conversations. Thanks for bearing with us as we figure it all out.

  • The Jira ticket was a wall of text and would have been more understandable with the addition of a UML diagram, provided here after our conversations. The latest push indicates that it did seem to be helpful in communicating what we were looking for. :)
  • @aerorahul Provided us a code example of the in the feature/scheduler branch. We will plan to use that as a guide for these types of tools as we grow our Factories.
  • In news unrelated to the implementation of the scheduler tool, we may need to look more closely at our pylint test. It may not be highlighting all the things we want...or it may not be finding all the python code. I will open a GH issue for that.

if 'stderr' in specs:
strings.append(f"{self._DIRECTIVE} --error {specs.stderr}")
return "\n".join(strings)
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 this section could be abstracted quite a bit to reduce duplicate information. Just spit balling, but I imaging something like (with the mod that I'm suggestion for map_flags above):

for flag, setting in self.map_flags.items():
    strings.append(f"{self._DIRECTIVE} --{flag}={setting}")

This takes advantage of the class already having these values set and the caller wouldn't need to pass them in again.


def map_flags():
mappings = {'account': '-A', 'partition': '-p', 'wallclock': '-t', 'job_name': '-J', 'nodes': '-N', 'ntasks_per_node': '-n', 'output': '-o', 'error': '-e'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, I think it could be useful to map in the other direction, so then you could map the value of the flag directly to the flag itself.

You might also consider making it an object attribute in the init method (instead of its own method here) so that you don't have to repeat the list of info, and all the values are stored in a convenient dict for retrieval elsewhere if needed.

These additions were ad hoc for discussion purposes. More changes will
be needed!
@christinaholtNOAA
Copy link
Contributor

I just pushed a small batch of changes after @ope-adejumo and I talked through some of the fuzzy concepts of what should/shouldn't be in the base class and sub class. We also discussed some of the implementation details of providing native flags, joining stderr/stdout among diff schedulers, and what it means to write a job card.

@christinaholtNOAA
Copy link
Contributor

We should probably close this PR.

@christinaholtNOAA christinaholtNOAA deleted the UW-44_Generate_generic_scheduler_tool branch April 14, 2023 16:47
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.

None yet

3 participants