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

Refactor staging functions #614

Merged
merged 1 commit into from
May 3, 2021
Merged

Refactor staging functions #614

merged 1 commit into from
May 3, 2021

Conversation

jenest
Copy link
Collaborator

@jenest jenest commented Apr 6, 2021

Description

Refactors common code in staging functions of unifyfs-rm.c into a generic_stage() function.

Motivation and Context

Addresses issue #516.

How Has This Been Tested?

Using tests 0700 and 9300 on Summit. Unfortunately, these tests fail for either dev or my own branch. However, @CamStan is not seeing these problems on LLNL systems.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Testing (addition of new tests or update to current tests)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the UnifyFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted.

@jenest jenest force-pushed the issue_516 branch 2 times, most recently from baf7356 to ff94eca Compare April 6, 2021 15:46
@CamStan
Copy link
Member

CamStan commented Apr 6, 2021

Not sure it has anything to do with this, but I recalled that there is one stage test currently set as test_might_fail, but I don't recall why this decision was made. Might have been a failure that only showed up on Travis and not locally and never got debugged on Travis.

test_expect_success "final output is identical to initial input" '
test_might_fail test_cmp ${UNIFYFS_TEST_TMPDIR}/stage_source/source_0700.file ${UNIFYFS_TEST_TMPDIR}/stage_destination_0700/destination_0700.file
'

Essentially, even if this test fails on Travis, it will show as passing, which is something we should probably adjust in the long run.

@jenest
Copy link
Collaborator Author

jenest commented Apr 7, 2021

I would probably agree that this should be adjusted, but I might let someone that has more experience with this code make the call for whether/how that should be done.

@jenest jenest changed the title Refactor staging functions - ignore me Refactor staging functions - draft Apr 12, 2021
@jenest
Copy link
Collaborator Author

jenest commented Apr 14, 2021

It might be best to make sure things are working so I can test my changes. Currently, tests 700 and 9300 are failing on Summit, even with the dev branch.

@MichaelBrim Thoughts on addressing this?

@MichaelBrim
Copy link
Collaborator

@jenest I would just open another issue to resolve problems that already exist.

@jenest jenest changed the title Refactor staging functions - draft Refactor staging functions Apr 16, 2021
@jenest
Copy link
Collaborator Author

jenest commented Apr 16, 2021

Referenced in a comment on issue #523. I will be happy to create a separate issue if needed.

@jenest jenest marked this pull request as ready for review April 16, 2021 13:14
Copy link
Collaborator

@MichaelBrim MichaelBrim left a comment

Choose a reason for hiding this comment

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

Now that we are generating a single c-string representation of the full launch command, it's no longer necessary to keep a separate c-string for n_nodes. Could you update the command snprintf() to just use the %zu format and pass it resource->n_nodes as an arg? This change would go in each of the xxxrun_stage() functions.

Fix formatting

Reformat changes
@adammoody adammoody merged commit 724d5b4 into LLNL:dev May 3, 2021
@jenest jenest deleted the issue_516 branch May 7, 2021 15:24
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