Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New _fetch_status code. #417
New _fetch_status code. #417
Changes from 44 commits
081f367
e048af6
4e32661
7855249
34b6cef
6138326
de73489
e06b74f
1fccff8
b88f62e
ef2d168
28916a1
1b2ab67
09a8b0b
03fbcf5
bd6a6a3
cc52517
ff63832
36ef3f9
3f5e04d
fd3c273
af03674
8b9b53c
eafa6cc
c196dfe
80da4d2
e5cf670
0f287d8
82dba08
fa6dad6
d367611
e268d43
aa8b327
65fd807
5a15f5b
234b879
a89137d
3fc8397
5288a25
c2086a4
f695fa6
95dc985
4e62a3d
bf58677
0256b57
d13e1e7
5774fc5
581a591
957c9ce
eb4d693
86824fb
f9ba946
9de6be8
d673995
85b25a7
d73a6b9
a66cb77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These appear superfluous to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I see where these are used now. I would rather set
fp.root_directory = lambda: PROJECT_DIRECTORY
here than include code in the main code base that exists just to facilitate this test. Is there a reason that wouldn't work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that won't work because we have to mock the root directory used for operation id generation separately from the real root directory that contains the jobs. We need consistently generated ids (using the mocked value) but we still need the root directory to be correct (pointing to some temporary directory during tests) or else we break signac (maybe instantiating jobs fails? Or the config is inaccessible? Or it can't pickle/unpickle? Something like that, but I forgot the exact reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, you're right that this won't work in that case. How about the following context manager instead:
We don't execute anything in parallel here, so I think this should be safe. It's a few more lines of code than the current solution, but I would really like to avoid injected this code into flow/project.py if at all possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this would be much better. I'll adapt this.