Feature/hooks track operation #739
Conversation
…tzerlab/signac-flow into feature/hooks-TrackOperation
klywang
requested review from
csadorf and
SchoeniPhlippsn
and removed request for
a team
for more information, see https://pre-commit.ci
…tzerlab/signac-flow into feature/hooks-TrackOperation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #739 +/- ##
==========================================
+ Coverage 69.08% 69.41% +0.32%
==========================================
Files 45 48 +3
Lines 4348 4407 +59
Branches 890 1065 +175
==========================================
+ Hits 3004 3059 +55
- Misses 1138 1140 +2
- Partials 206 208 +2 ☔ View full report in Codecov by Sentry. |
flow/hooks/track_operations.py
Outdated
| Examples | ||
| -------- | ||
| The following example will install :class:`~.TrackOperations` at the operation level. | ||
| Where the log will be stored in the job document. |
There was a problem hiding this comment.
I'm not sure what is meant here. Is this a complete sentence?
tests/test_project.py
Outdated
| ): | ||
| assert metadata["stage"] == expected_stage | ||
| # When running on MacOS CI a private/ is prepended to job.project.path | ||
| # seemingly indeterminetly. If metadata["project"]["path"] is checked |
There was a problem hiding this comment.
| # seemingly indeterminetly. If metadata["project"]["path"] is checked | |
| # seemingly indeterminately. If metadata["project"]["path"] is checked |
tests/test_project.py
Outdated
| repo, | ||
| ): | ||
| assert metadata["stage"] == expected_stage | ||
| # When running on MacOS CI a private/ is prepended to job.project.path |
There was a problem hiding this comment.
This is rather surprising behavior. I would encourage finding a way to have deterministic tests.
There was a problem hiding this comment.
I thought about this a bit more. I would xfail or skip this test on macOS rather than introduce a lot of workarounds in the testing code.
tests/test_project.py
Outdated
| else: | ||
| if self.error_on_no_git: | ||
| # Should not happen in actual testing, here for developers. | ||
| assert False, "Expected gitpython for test." |
There was a problem hiding this comment.
This doesn't seem right. What is "actual testing"? Do you mean CI is "actual testing" but local runs of the test suite are not? It is okay to require that developers have gitpython installed to run the test suite, or you can edit tests requiring gitpython to call pytest.importorskip.
There was a problem hiding this comment.
We can assume that the git package is present during testing since we skip the tests on the import error.
There was a problem hiding this comment.
It isn't testing in any case. It is there to ensure we never reach this point. It should be impossible to get here. If that isn't the case there is an error in the test.
Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Bradley Dice <[email protected]>
Doesn't use pytest.xfail or skip since only one check fails.
|
@b-butler Do you think you will be able to merge this soon? |
Also fix documentation errors. Co-authored-by: Bradley Dice <[email protected]>
Implemented builtin
TrackOperationshooks.TrackOperationskeeps detailed metadata records about the project directory and operations as a log file, optionally in conjunction withgit(#637).Description
Added
TrackOperationsutilities tosignac-flowhooks and added tests. Some of these tests requiregitpython. We made this an optional dependency for testing since there are no other tests insignac-flowwhich requiregitpython.In the original implementation of
TrackOperations(#189), operation directives were recorded in the metadata. However, our current implementation ofhookstakes inoperationas a string andjobassignac.job.Job. Does anyone know how we could access directives in the current implementation of hooks?Motivation and Context
TrackOperationsare a useful way to track the state of your code when a specific job operation is executed.Checklist: