Conversation
javierggt
force-pushed
the
no-pexpect-2
branch
from
c3a9fde to
e02ebe9
Compare
javierggt
force-pushed
the
no-pexpect-2
branch
from
17d3c8b to
1955b7d
Compare
javierggt
force-pushed
the
no-pexpect-2
branch
from
a958526 to
bc26055
Compare
|
As a drop-in replacement for ska_shell, this didn't work with my local copy of runasp.py So I think we'd need to finish sot/runasp#8 or just add some test cases that are doing the things that are in the current runasp.py. |
|
I think we should make this work with runasp. It's not clear to me from the error what is the problem. |
|
It looked to me like it just didn't run the command with the PATH from the ascds_env to find punlearn. But I think your changes for runasp made sense anyway with regard to improved logging too, so maybe a compromise PR in runasp? |
|
it seems like the exception is raised because the command returned a non-zero code. Note that the original docstring says:
So that would be the original intended behavior anyway. The question is: is the |
|
The current changes in this PR can be used instead of that PR in runasp (that was my intention). The functional test in this PR is the way I would use it in runasp. Still, I did not expect the runasp master to fail. It shouldn't, unless there is some issue in master that I inadvertently fixed. |
|
It looks like my error trace copy-paste didn't have the Command not founds This is straight from master. |
|
|
jeanconn
left a comment
jeanconn
left a comment
There was a problem hiding this comment.
As noted, this didn't work in my limited testing.
|
It looks like for me a minimal working reproducible example of the runasp test goes something like:
And that fails with |
|
I cannot reproduce your error. I get a different failure, and it does not look like a shell issue. I followed your instructions and created an environment in I can clearly see the In my case, it fails with: It succeeds with ska_shell master, so whatever the issue is, it is introduced by this branch, but it is not obvious to me. |
|
Does your and the code in /export/jgonzale/miniconda3/envs/ska-shell/lib/python3.12/site-packages/ska_shell/shell.py also has diffs relative to the shell.py I've got. |
|
Yes, it matches: I ran the pipeline using ska_shell master and it succeeds. However, I do see this in the output: This means I ran it ignoring the return code (this needs changes in ska_shell and runasp), and it ran to the end, but I don't know what failed and what succeeded. |
|
I just pushed a change that adds an exception type that is used when raising exception if the return code is non-zero. Then in runasp I added |
|
@jeanconn does this gist fail for you? https://gist.github.com/javierggt/4296f32b6ba2223030e944ecf70a39b3 |
|
The source of the problem is that |
|
but of course this will fail if the given command has single quotes. |
|
After determining that the issue was the Using this branch is it is currently, that error does not occur anymore. I did some tests with commands using single quotes and could not get it to fail, so I think I prefer this PR over the ska_helpers one. The pipeline still fails with this error: I checked that this error also happens when using the latest ska_shell release, but ska_shell does not bail out. I contend that this is an actual error, and ska_shell should fail in this case. One could add a |
|
Hi @javierggt - would you please update the Description to document that you ran the unit tests? |
|
Also, for my use case in runasp it looks like I want to continue on error status 80. How can I get the status code of the last error from this code to make that decision? |
I didn't want to make that an option because it's not a good behavior. You could just run with |
|
Yes, but I have no control over that behavior unless I doubly-wrap the pipe manager. So how would I access the error code? |
|
Right now you are not checking the return code behavior, but that runasp gives the same result as a pipeline that ran "successfully". Maybe you could use the branch in sot/runasp/pull/9? I think this is something that should be fixed in runasp one way or another. |
|
Right, I mean I understand that previously nothing was being checked, but now that I've looked at the situations that return non-zero it seems like we could do better than that. Anyway, I simultaneously edited runasp to check most calls except ones that are going to end in code 80 and it is working for me. |
|
I figured that just adding the |
|
One thing one could add is that the exception that is raised includes the error code. Right now it is just included in the message that says "Shell command %s failed with exit status %d". |
|
sorry, my mistake. In this branch, the error code is not included in the message, but I could include it, and the return code could be a member of |
|
I agree that adding the "check" is doing better. But I don't really agree with "Strictly speaking, you do not need to know the last return code," . I know that return codes are all over the place and generally you just need to stop on a non-zero value. But if I've identified a specific case where one value seems to be benign for a certain constructed cmd , I don't really see a way to handle that correctly without knowing the value (except to add a another shell script to wrap flt_run_pipe and conditionally mangle the exit status). But you'll see in my PR on runasp that I just set check conditionally to not check for the case that is presently failing but let it check for all the rest. |
|
Regarding "but I could include it, and the return code could be a member of ShellError" that would be awesome! And you see from email traffic I'd also like to get the upstream flt_run_pipe code fixed if it is fixable. |
2 participants
Description
This PR removes uses of pexpect in ska_shell.
It also adds a
checkargument to several functions.check=Truecauses an exception if any command returns a non-zero code.check=Falseruns all commands even if one fails and no exception is raised.getenvdoes not have thecheckargument. The current behavior in master varies, but I have seen cases where it does not raise an exception (even though the docstring says it should).It also does some small unrelated changes:
test_errorwhich for some reason triggered a failure when running testr. I don't know why this did not fail before and failed now, but it does not hurt to just change the name.Notes
A small complicating issue was that the tcsh call was always loading
.cshrc, and that was overwriting the PATH given as input. To sidestep this, the call to tcsh is wrapped in a call to bash, and then bash calls tcsh with the-foption.This PR was motivated by warnings appearing in tests that were caused by pexpect (mica and agasc).
An alternative is #30, where I only removed uses of pexpect in the
getenvfunction.Interface impacts
checkargument torun_shell,bash,bash_shell,tcsh,tcsh_shellrun_shell. The logger is called continuously, so you get a log as the program progresses. This is useful for long-running processes. I used this in runasp.Testing
Unit tests
Independent check of unit tests by Jean
Functional tests
After running the tests linked in the description, I installed this branch into that environment and ran
and the warning in agasc went away (not the one in kadi).
JC ran the aimpoint code that calls dmcoords and it did not fail