Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Missing perl error message #322

Closed
wants to merge 6 commits into from
Closed

Missing perl error message #322

wants to merge 6 commits into from

Conversation

dirkgr
Copy link
Member

@dirkgr dirkgr commented Feb 23, 2022

@dirkgr dirkgr requested a review from AkshitaB February 23, 2022 23:34
@dirkgr
Copy link
Member Author

dirkgr commented Feb 28, 2022

There is now nothing left of this PR, but thanks to squash merging, GitHub can't figure this out automatically.

@dirkgr dirkgr closed this Feb 28, 2022
@Denbergvanthijs
Copy link

Denbergvanthijs commented Mar 1, 2022

Thank you very much! Just a quick suggestion: wouldn't from shutil import which be a cleaner option instead of a try-except? It looks to be multiplatform and available since Python 3.3. When the executable is not found, it returns None.

@dirkgr
Copy link
Member Author

dirkgr commented Mar 1, 2022

While unlikely, that introduces a race condition. What if someone moves the perl executable after your call to which and before the call to subprocess.run?

@Denbergvanthijs
Copy link

That would indeed be a possibility, thank you for your answer!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear error when Perl is not found in path when training AllenNLP Semantic Role Labeling model
3 participants