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 option to generate LM image and GC via two separate jobs #446
base: main
Are you sure you want to change the base?
Add option to generate LM image and GC via two separate jobs #446
Changes from 12 commits
620a042
a68265c
b74c654
79cbd2a
78f6bed
291b734
d209e8f
02d7088
6a236c8
a61b03f
008c9dc
07e1136
cdc791a
ab010c5
5a8ec3d
ed001c4
115b160
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.
I think I would either like to spell config and post_config out or have a small docstring here.
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 agree with @Atticus1806
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 agree with @Atticus1806
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.
Is the post config also
rasr.RasrConfig
? Or some other type?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, post configs should also be of type RasrConfig
TODO: add
Optional
import above and check with black for line length :PThere 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.
Is
.create_config
only called within this job? Otherwise we need to be careful with changing the returned variables. But I think you caught the cases here and otherwise the change is easy.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.
the
AdvancedTreeSearchWithRescoringJob
below in this file inherits fromAdvancedTreeSearchJob
and usessuper().create_config
. that should be fixed.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.
The function
lm.find_arpa_lms
only returns the LMs that do not already have an lm.image - because for those that already have an image we do not need to create a new one.But the lm.image usually is defined in the post_config. When we here not pass the post config, then all (arpa) LMs are found and returned. Therefore we do extra work here.
And even worse: below in line 404/418 we call the function again, but with the post config. So if the original crp contain a mix of arpa LMs out of which some have already images and others do not, then the items in
arpa_lms
differ between the calls. And since we use the index to match the image to the LM, the mapping will be off and the wrong image will be assigned.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.
Is there a reason for the
_
. Maybe I am overlooking something in the web view. If its not used you could just fully replace it with_
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 think the `post_config here is unused, because
config
andpost_confic
are extracted from thecrp
config
is modifiedcrp
with the oldpost_config
is still being used+1 to can be just
_
to make clear it is unusedThere 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 think the `post_config here is unused, because
config
andpost_confic
are extracted from thecrp
config
is modifiedcrp
with the oldpost_config
is still being used+1 to can be just
_
to make clear it is unusedThere 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.
We already have called lm.find_arpa_lms above. Maybe move the above call out of the if condition and then reuse the
arpa_lms
from above here. This would also avoid mismatching items in the list as I outlined above.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.
Same as above