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
Add getters and setters for Task priority in AlchemiscaleClient #213
Add getters and setters for Task priority in AlchemiscaleClient #213
Changes from 14 commits
5583eea
6208602
5a5165b
8a191ff
5c8ab2c
3641247
c89f05a
4702ba3
3695ce1
6a2bb43
cf587a9
8d7dce5
20f5600
3dda058
d540476
5edcb14
011e8cc
48a4a26
cd88f81
139d03d
9e22c8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you add a note here on what kind of integers are allowed for
priority
, and what these values mean? As-in, priority1
is the highest priority,2
the next highest, and so on, with the lowest priority allowed being2**64 / 2
(max value of java long, since this gets stored in Neo4j).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.
I was wrong about this; max value of a java long is
2**64/2 - 1
, or2**63 - 1
. Fixing this myself now.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.
Also adding guardrail in
Neo4jStore
.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.
Just to simplify a bit and make it consistent with other methods, let's axe this method's ability to take a single task, since it avoids the asymmetry of input sometimes being a non-iterable but output always being an iterable.
We generally try to take this pattern at this lowest layer of the stack; it doesn't need to be user-friendly, but we aim for a fairly logical consistency where possible.
This will simplify the tests for these methods as well, reducing the input space.
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.
Can you also add
-> List[Optional[ScopedKey]]
as the output?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.
And rename
task
totasks
. 😁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.
Can you also add a docstring?
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.
Agreed. This will require changing compute tests using the single task inputs. I was trying to preserve previous functionality. Might be worthwhile doing a general consistency sweep before the next release.
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.
Please add a docstring here as well!