-
Notifications
You must be signed in to change notification settings - Fork 41
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 command line interface to allow scripts to be run from sasview.exe #2280
Conversation
Making the shipped It would be nice to see:
A functional test of the installed version is also needed - that can build on #2263 once merged. Running those same I'd suggest writing the functional tests as a series of tests with |
This PR seems to be a part of bigger discussion that we plan to have tomorrow at the code camp. |
Depending on the outcome of a Windows review of this PR a small doc change may be needed to sasmodels/doc/guide/scripting.rst. See SasView/sasmodels#526 (comment) for details. The doc is currently assuming the command will be 'sasview', not 'SasViewCom', or both. |
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've tested this PR for functionality and it works as exptected. I will look at the code later
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.
Functionality review on Windows x64/W10.
sasview
Start sasview gui
Works.
sasview script [args...]
Run script with sasview libraries available
Works. (But script did not require args to be passed)
sasview -c "python statements"
Execute python statements with sasview libraries available.
I tried sasview -c "print("hello world")" but this seemingly did nothing. Did I misunderstand what this should have done?
sasview -m module [args...]
Run module as main.
Not tried.
sasview -i
Start ipython interpreter.
@smk78 thanks for the report. When you ran the script how do you know that it ran? Did it produce any output on the console? To check the argument processing you can include: If your We may need to attach a windows console when running from the exe (docs). Something like (untested): import os, sys
if os.name == 'nt': # Make sure we are attached to a console
from win32 import win32console as con
if con.GetConsoleWindow() == 0: # No console attached
con.AllocConsole()
con.SetConsoleTitle('SasView console')
sys.stdin = con.GetStdHandle(con.STD_INPUT_HANDLE) # -10
sys.stdout = con.GetStdHandle(con.STD_OUTPUT_HANDLE) # -11
sys.stderr = con.GetStdHandle(con.STD_ERROR_HANDLE) # -12 This creates a new console for the interaction. I don't know if we can attach to the existing console so that we can interact with shell pipelines. |
Once we have a working console you can try Or try |
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.
@pkienzle , there was no output to the console when the script ran, but I knew that it had run because the script (from @wpotrzebowski) was designed to produce a plot and it did.
Similarly:
sasview -m sasmodels.compare sphere
With your sys.argv addition our script looks like this:
from matplotlib import pyplot as plt
from sasmodels.core import load_model
from sasmodels.direct_model import call_kernel
from numpy import logspace
import sasmodels
import sys
print("Input arguments:", sys.argv, file=open('script.out','w'))
model=load_model('cylinder')
q=logspace(-3,-1,200)
kernel=model.make_kernel([q])
Iq=call_kernel(kernel,dict(radius=200))
plt.loglog(q,Iq)
plt.xlabel('q (1/A)')
plt.ylabel('I(q)')
plt.title('Cylinder with radius 200')
plt.show()
and on running it script.out contains:
Input arguments: ['wojtek4.py']
It then occurred to me that you can redirect STDOUT from the command line, so I tried it with your bumps help example:
sasview -m bumps.cli > bumps.out
for which bumps.out contained (encouragingly):
Usage: bumps [options] modelfile [modelargs]
The modelfile is a Python script (i.e., a series of Python commands)
which sets up the data, the models, and the fittable parameters.
The model arguments are available in the modelfile as sys.argv[1:].
Model arguments may not start with '-'.
Options:
--preview
display model but do not perform a fitting operation
--pars=filename or store path
initial parameter values; fit results are saved as path/<modelname>.par
--plot=log [linear|log|residuals]
type of plot to display
--trim=true
trim any remaining burn before displaying plots [dream only]
--simulate
simulate a dataset using the initial problem parameters
--simrandom
simulate a dataset using random problem parameters
--shake
set random parameters before fitting
--noise=5%
percent noise to add to the simulated data
--seed=integer
random number seed
--err
show uncertainty estimate from curvature at the minimum
--cov
show the covariance matrix for the model when done
--entropy=gmm|mvn|wnn|llf
compute entropy on posterior distribution [dream only]
--staj
output staj file when done [Refl1D only]
--edit
start the gui
--view=linear|log
one of the predefined problem views; reflectometry also has fresnel,
logfresnel, q4 and residuals
--store=path
output directory for plots and models
--overwrite
if store already exists, replace it
--resume=path [dream]
resume a fit from previous stored state; if path is '-' then use the
path given by --store, if it exists
--parallel=n
run fit using multiprocessing for parallelism; use --parallel=0 for all cpus
--mpi
run fit using MPI for parallelism (use command "mpirun -n cpus ...")
--batch
batch mode; save output in .mon file and don't show plots after fit
--noshow
semi-batch; send output to console but don't show plots after fit
--time=inf
run for a maximum number of hours
--checkpoint=0
save fit state every n hours, or 0 for no checkpoints
--fit=amoeba [amoeba|de|dream|lm|newton|pt|scipy.leastsq]
fitting engine to use; see manual for details
--steps=0 [amoeba|de|dream|lm|newton|pt|scipy.leastsq]
number of fit iterations after any burn-in time; use samples if steps=0
--samples=1e4 [dream]
set steps=samples/(pop*#pars) so the target number of samples is drawn
--xtol=1e-4 [de, amoeba]
minimum population diameter
--ftol=1e-4 [de, amoeba]
minimum population flatness
--alpha=0.0 [dream]
p-level for rejecting convergence; with fewer samples use a stricter
stopping condition, such as --alpha=0.01 --samples=20000
--outliers=none [dream]
name of test used for removing outlier chains every N samples:
none: no outlier removal
iqr: use interquartile range on likelihood
grubbs: use t-test on likelihood
mahal: use distance from parameter values on the best chain
--pop=10 [dream, de, rl, ps]
population size is pop times number of fitted parameters; if pop is
negative, then set population size to -pop.
--burn=100 [dream, pt]
number of burn-in iterations before accumulating stats
--thin=1 [dream]
number of fit iterations between steps
--nT=25
--Tmin=0.1
--Tmax=10 [pt]
temperatures vector; use a higher maximum temperature and a larger
nT if your fit is getting stuck in local minima
--CR=0.9 [de, rl, pt]
crossover ratio for population mixing
--starts=1 [newton, rl, amoeba]
number of times to run the fit from random starting points.
--keep_best
when running with multiple starts, restart from a point near the
last minimum rather than using a completely random starting point.
--init=eps [dream]
population initialization method:
eps: ball around initial parameter set
lhs: latin hypercube sampling
cov: normally distributed according to covariance matrix
random: uniformly distributed within parameter ranges
--stepmon
show details for each step
--resynth=0
run resynthesis error analysis for n generations
--time_model
run the model --steps times in order to estimate total run time.
--profile
run the python profiler on the model; use --steps to run multiple
models for better statistics
--chisq
print the model description and chisq value and exit
-m/-c/-p command
run the python interpreter with bumps on the path:
m: command is a module such as bumps.cli, run as __main__
c: command is a python one-line command
p: command is the name of a python script
-i
start the interactive interpreter
-?/-h/--help
display this help
@smk78 please try your modified wojtek4.py with I'm surprised that the Meanwhile, can you try the following import os, sys
if os.name == 'nt': # Make sure we are attached to a console
from win32 import win32console as con
if con.GetConsoleWindow() == 0: # No console attached
con.AllocConsole()
con.SetConsoleTitle('SasView console')
sys.stdin = con.GetStdHandle(con.STD_INPUT_HANDLE) # -10
sys.stdout = con.GetStdHandle(con.STD_OUTPUT_HANDLE) # -11
sys.stderr = con.GetStdHandle(con.STD_ERROR_HANDLE) # -12
import code
code.interact(local={'exit': sys.exit}) This should start a new console with a python prompt which will I hope go away when you type |
If you don't need input you might be able to pipe to type: sasview -m bumps.cli | type This might not work (I don't know if |
Yes, it does!
As expected, if I run this as python test_cli.py it invokes Python (in the same console) and I can exit(). If I run this as sasview test_cli.py this happens: According to pip list pywin32 exists in both (versions 223 and 304, respectively):
|
The error says that win32 is not included in setupSasview.exe. I fiddled the .spec file so maybe it will be included now. |
@pkienzle , I just downloaded and installed the latest artefact for this branch. I get a slightly different error this time: |
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.
Promising work :)
There's a few comments I've made inline. There's also three general areas to address:
- unittesting of the new code
- functional testing of the new commandline interface
- documentation of the commandline interface
installers/sasview.spec
Outdated
@@ -65,6 +66,10 @@ hiddenimports = [ | |||
'uncertainties', | |||
] | |||
|
|||
if os.name == 'nt': | |||
# Need win32 to run sasview from the command line. | |||
hiddenimports.append('win32') |
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 'win32' module is commonly installed but not actually in stdlib from what I can see. Should it be added to the requirements.txt
and CI installation steps?
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.
btw reading pyinstaller/pyinstaller#6244 makes me wonder if this is actually going to be a solvable problem on windows.
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.
Looking at the win32 api it seems we should be able to attach to an existing console, so it is just a matter of finding the right one. Worst case we need to create a SasViewCom.exe for windows to run scripts.
installers/sasview.spec
Outdated
@@ -65,6 +66,10 @@ hiddenimports = [ | |||
'uncertainties', | |||
] | |||
|
|||
if os.name == 'nt': |
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.
Elsewhere in the spec
file, platform.system()
is used. Is there compelling reason to use os.name
here and not platform.system()
? https://docs.python.org/3/library/platform.html#platform.system
run.py
Outdated
import bumps | ||
except ImportError: | ||
addpath(joinpath(root, '..', 'bumps')) | ||
# TODO: Do we prioritize the sister repo or the installed package? |
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 wish we didn't do any manipulation of PYTHONPATH.... lots of problems would go away if we stopped doing this. But that's all for another PR in another lifetime.
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.
Out of curiosity, where/when is PYTHONPATH set. I don't have it set...
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.
@smk78, run.py
is full of calls to addpath
which is a function that is fiddling with PYTHONPATH
(and/or sys,path
). The custom test runners used to have lots of that too and they have been slowly retired, which helps.
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 point of run.py is to set up the python when running in development mode. I often set up parallel directories for different branches and don't want to set up a complete python environment when I do so. This will be increasingly the case if we go to a fork rather than branch workflow for PRs.
I would think you will always want the equivalent for testing. How else do you run the tests before installing?
import code | ||
code.interact(local={'exit': sys.exit}) | ||
|
||
def main(logging="production"): |
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.
main
doesn't return but seems that it might sys.exit()
from deep within. Making it so that the only way out of the function is to return an integer (that becomes the exit code) is often nicer - it's a more modular design, it's more easily tested, and the exit points are simpler to understand.
Likewise, passing the commandline args into it as a list of strings can make it easier to drive it for testing.
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.
This function needs to hack sys.argv
to get things like bumps and ipython to work properly, so passing in args would be something of a fiction. I could write a context manager with push_argv(args):
to restore on exit, but then so could the hypothetical test harness, so I'll leave it as is until needed.
from sas.qtgui.MainWindow.MainWindow import run_sasview as run_gui | ||
run_gui() | ||
|
||
if __name__ == "__main__": |
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.
What's the purpose of this entry point as opposed to run.py
and installers/sasview.py
. This seems like a good opportunity to reduce the number of entry points into the code; ideally there would be only 1 for ease of maintenance.
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.
This is the real entrypoint. run.py
and sasview.py
prepare the environment for running.
In future when we allow pip install sasview
then we can start it with python -m sas.cli
. This can be handy from shell scripts and make where you replace python
with $PYTHON
or sys.executable
in python scripts. I've used this style for a number of applications such as sphinx, nosetest and ipython. It's also handy when "borrowing" an interpreter from an environment.
Maybe we should have sas/__main__.py
so that we can use python -m sas
to run sasview?
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.
Either this is an unneeded __main__
that no-one should call, or it is yet another __main__
. sasview.py and run.py aren't calling this entry point, so this is adding another entry point. Reducing the entry points is good as you say, but this isn't doing so... yet.
I'm cautious of scope creep - this PR is not about fixing the bug that there is a multiplicity of ways of starting sasview. I'm also cautious about adding yet another way of starting sasview to the set, which then needs testing and maintenance.
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.
run.py
is not a published user interface. As a developer tool it doesn't need testing and maintenance. That is, if it is broken the developer who cares can fix it. FWIW, it has been broken since sasdata was split off and nobody has complained, so maybe I'm the only one using it. If it goes away I'll drop the equivalent into ~/bin like I currently do for sasmodels.compare.
That leaves the $ sasview.py
which does:
... freeze nonsense that is required for multiprocessing on windows bundled installers ...
from sas.cli import main
sys.exit(main())
and $ python -m sas.cli
which does:
if __name__ == "__main__":
sys.exit(main())
I find that the later is more reliable since it goes through the particular python interpreter, not whatever sasview.py
happens to be on the path. Indeed, the python docs give python -m pip ...
as the documented interface to pip.
I'll grant that this will be more useful when we have a sasview package on pypi. Until then it requires sasview on the path, and anyone who knows how to do that can call the entry point themselves. Hint:
PYTHONPATH=/path/to/sasview/src:... python -m sas.cli ...
turns into
PYTHONPATH=/path/to/sasview/src:... python -c "import sas.cli; sys.exit(sas.cli.main())" ...
My inclination is to leave it in, but I won't object if it gets removed.
@llimeht commented:
Documentation is being handled in SasView/sasmodels#526 |
That obviously needs to grow to include the things other than |
Running the following as test_cli.py:
|
More progress. The popup console hangs around until the user dismisses it, giving them a chance to read the output. Code is in It is still not complete. The popup console is not recognized as the windows console so the python interactive loop doesn't work properly. In particular, Also, despite using lazy console creation (only on read/write from stdin) the console window was popping up when running sasview.exe as a GUI. It should only be appearing if the program reads from or writes to the console. Because of the above problems I don't enable it by default. Instead scripts need to do the following:
A better solution is to create a separate console app (sasbatch.exe?) so this hack won't be needed. |
Ok, so post todays call have just taken artefact https://github.com/SasView/sasview/actions/runs/4267800941 for a drive on W10/x64. Before doing so I temporarily renamed sasview.exe and then typed 'sasview' just to make sure that this command wasn't going to invoke any other version on my machine. It did not.
Works! Invokes the GUI.
Does nothing!
Works! Writes the help to bumps.out
Works! Generates a plot.
Works and apparently accepts the extra arguments (what it did with them is another matter)
Works! Generates a plot.
But, interestingly:
Is accepted but does nothing! (Notice the change of quotes around the string from "" to '')
Works! And 'hello word' is written in to statement.out
Does nothing!
Works! And the version is written in to version.out And the big one...
So this is all looking quite favourable again, apart from starting the interpreter and the nuisance of redirecting STDOUT. |
Yes, this is expected behaviour because I'm not calling I added a new option
[updated] You can do plots, but don't use
The fix will be to somehow trigger Since the |
Looking at the CPython code for input we also need to define In particular, CPython checks that the fileno for python stdin matches that for the stdin from the C library. I'm betting that the windows app startup process leaves these empty so we may have to hack msvcrt to set them to the console buffer, but with luck it will be as easy as the following:
[update] The above is no better than the existing solution. CPython code for Docs for msvcrt: https://docs.python.org/3/library/msvcrt.html |
I added the We wouldn't need the option if it only attached the console when something is written to it. I tried this but I found that it was popping up even when nothing was written. I didn't spend any time trying to debug it. |
Are we ready to merge this then?! @smk78 and others? It sounds like it is working even better than at code camp which we said was "good enough" ? |
I would say from a functionality perspective, yes. But I guess we need to hear from @pkienzle . Also @llimeht had quite a few comments. I don't know how many of those are still valid/current? |
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.
Code looks fine. Works as expected.
Allow scripts from the command line:
Partially addresses #2237. Replaces #2278.
We could also add a --jupyter command line option which opens up a server on localhost with everything setup. It'll increase the size of our installed package though, especially if we include jupyterhub with a nodejs distribution.
I don't have a windows box handy so couldn't test the installed package on windows.
Do we need multiprocessing freeze support?
I don't know how modern qt apps handle console vs. gui support. Maybe need to do something like creating a separate sasviewcom.exe for the console.