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 the recipe parser #119

Merged
merged 22 commits into from
Jun 3, 2019
Merged

Refactor the recipe parser #119

merged 22 commits into from
Jun 3, 2019

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented May 29, 2019

This is a work in progress to refactor the recipe parser to have a better design, and more easily switch between types. Specifically:

  • a simple base class for a recipe spython.main.parse.recipe.Recipe is a skeleton that holds fields to describe a container recipe (environment, entrypoint, etc.)
  • a module folder of parsers spython.main.parser.parsers will instantiate and fill a Recipe from a recipe type they are specified for (e.g., right now we have DockerParser and SingularityParser
  • a module folder of writers spython.main.parser.writers will take a populated Recipe and use it to write a recipe to file (e.g., again we have DockerWriter and SingularityWriter

There are a lot more tests, and work to be done, but I need to run and want to get this code into a PR for others to mull over before I need to leave! Work remaining to be done:

  • ensure that testdata (or another name for the folder) is added to the install
  • write tests to parse in specific files and then check for exact output (a testFiles or similar folder)
  • update / revamp docs to reflect new organization and usage!

I'd like the PR #117 to go in first so that the testing structure is better set up, and then we can work here. I don't expect tests to pass yet because the testdata is not properly added.

These should be great changes! I'm happy to tighten up the logic / organization a bit to allow for many more writer and parser types.

This will close:

vsoch added 3 commits May 28, 2019 18:28
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
… how to add test data

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch vsoch changed the title [WIP ]Refactor the recipe parser [WIP] Refactor the recipe parser May 29, 2019
@vsoch
Copy link
Member Author

vsoch commented May 29, 2019

This will close #111 (comment)

vsoch added 11 commits May 29, 2019 11:21
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch vsoch changed the title [WIP] Refactor the recipe parser Refactor the recipe parser May 30, 2019
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented May 30, 2019

This will also close #70, a Recipe class can be export to json with recipe.json(). I've done substantial work for the linting, but I want to do a bit more so I'm not closing the issue.

@Flamefire this is ready for your review! I've also updated the docs for interacting with recipes, parsers, and writers, and have rendered a (partial) version here:

singularity-python-recipes.pdf

or you can check out the markdown included here. The one testing setup I didn't create is this point (not crossed out above):

  • write tests to parse in specific files and then check for exact output (a testFiles or similar folder)

However I did tests across the modules. I'd like to suggest that we get this PR in first, and then move the detail work to another issue/PR with continued discussion.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
vsoch added 2 commits June 1, 2019 11:28
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Jun 1, 2019

These file comparison tests are too fragile - they pass for me locally, but not in CI.

@Flamefire
Copy link
Contributor

These file comparison tests are too fragile - they pass for me locally, but not in CI.

I wrote a comment on the line where and why it fails and how to solve it.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Jun 1, 2019

The issue was hard coded paths - it was added to expandPath(source) and expandPath(dest) because Singularity has a bug that doesn't support "." or "*" however this would return paths from the user system in the recipe, which is not appropriate for reproducibility. If this happens and the user wants to hard code a path, he or she can edit the recipe. The correct fix is not a hack here, but a fix of the bug upstream.

@vsoch
Copy link
Member Author

vsoch commented Jun 1, 2019

All set! I'd like to suggest (given the size of this PR) that the skeleton / core is reviewed, and detail changes (linting and more tests) are added after. The current with newly rendered python docs is harder to review in detail.

Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Did a cursory review of this PR. But it is to big to properly review it and contains many unrelated changes.

I suggest:

  • Split into 3 PRs: Linter, Docu, Recipe
    • Note that e.g. yapf can do most of those linting-related changes automatically. Only problem is conflicts on existing changes.
  • Combine commits with fixes of them etc to have a clean history (git rebase -i)

No need to rush out releases. Let's do this properly with good test coverage of the "new" features over multiple, clean PRs. Then release.

.circleci/config.yml Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
docs/api/_sources/changelog.md.txt Show resolved Hide resolved
spython/main/parse/parsers/docker.py Outdated Show resolved Hide resolved
spython/main/parse/parsers/docker.py Outdated Show resolved Hide resolved
spython/main/parse/parsers/docker.py Show resolved Hide resolved
spython/main/parse/parsers/singularity.py Outdated Show resolved Hide resolved
spython/main/parse/parsers/singularity.py Outdated Show resolved Hide resolved
@vsoch
Copy link
Member Author

vsoch commented Jun 2, 2019

@Flamefire the PR is huge and I've never done a rebase like that. If you can walk me through the steps (as I'm likely to mess it up) I can give it a try, otherwise our best option is to review this one PR here. And take a lesson for next time to not let it get so big.

vsoch added 3 commits June 2, 2019 10:48
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch vsoch merged commit 2c4d2b5 into master Jun 3, 2019
@vsoch vsoch deleted the refactor/recipe-parser branch June 3, 2019 14:40
@vsoch vsoch mentioned this pull request Jun 6, 2019
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.

2 participants