Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding command line interface to model #31

Merged
merged 7 commits into from
May 19, 2020

Conversation

ericbarefoot
Copy link
Collaborator

馃摚 New Feature 馃摚

Per thoughts from @amoodie, I'm adding here a set of interfaces into the pyDeltaRCM model that allow users to interface through the command line.

The two new basic ways to do this are
(1) through a command-line entry point and
(2) through __main__.py. Both are demo-ed below.

# interface 1
run_pyDeltaRCM                                  # runs the model using default values
run_pyDeltaRCM --version                        # returns the version
run_pyDeltaRCM --config <path_to_config_here>   # runs the model using a config file

This method works by establishing a console_scripts entry point in the setup.py file, so when the user pip installs, they get a command line hook. (see this line)

# interface 2
python -m pyDeltaRCM                                  # runs the model using default values
python -m pyDeltaRCM --version                        # returns the version
python -m pyDeltaRCM --config <path_to_config_here>   # runs the model using a config file

This method works by simply calling the same command line script function in a __main__.py file, so the two are essentially just aliases for each other.

Other new things

In addition to the main feature, to make this work as a default, I included a new timesteps parameter in the default.yml, which lets the user specify timesteps for the simplest case of running the model with no special stuff.

I also added a new test.yml because I'm not really sure how to test stuff from within the command line, so I figured a good way would be to output a file, and check that file. In the future, we can think about having this file be data output, and if necessary, load it and a saved version and check for congruence.

This PR mostly fixes #22, but doesn't include the last component, which is the matrix expansion of defaults. I suggest we postpone that, and make it a new issue.

I agree with Andrew, that the command line would be a good way to 
interface with the model, especially if configuration files could be 
passed from the command line.

In this commit, I add that feature using a console_scripts entrypoint in 
setup.py to a special new function called run_model, and this function 
runs the model for a given number of timesteps. This number of timesteps 
is set by the config file, which now accepts an additional timesteps 
parameter.
Basically, since we don't get a model instance to work with when we test 
from command line, I just hacked this together to save an output image, 
and check if that image exists. This is enabled with a new 
test_output.yaml file.

I also added a test to make sure the versions match when we query from 
command line.
now execution from a config file can be initiated either from a bash 
entrypoint or from a main file when called via 'python -m pyDeltaRCM' 
This change addresses DeltaRCM#22.
@ericbarefoot ericbarefoot added the enhancement New feature or request label May 18, 2020
@ericbarefoot ericbarefoot added this to the DeltaRCM on PyPi milestone May 18, 2020
@ericbarefoot ericbarefoot requested a review from amoodie May 18, 2020 17:05
Issue was with the subprocess package. I was using kw arguments that 
were introduced in 3.7, so I reverted the call to the 3.6 version, now 
it should run on both.
Copy link
Member

@amoodie amoodie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks neat @ericbarefoot! I made a few suggestions below, mostly about imports, since this is mostly importing code lol.

I think the api is top notch, if we decide to do a different hook down the line (as discussed), I think the api would be unchanged, so I'm good with all the high level stuff.

pyDeltaRCM/command_line.py Outdated Show resolved Hide resolved
pyDeltaRCM/command_line.py Outdated Show resolved Hide resolved
pyDeltaRCM/command_line.py Outdated Show resolved Hide resolved
pyDeltaRCM/command_line.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@amoodie
Copy link
Member

amoodie commented May 19, 2020

also forgot to comment on:

I also added a new test.yml because I'm not really sure how to test stuff from within the command line, so I figured a good way would be to output a file, and check that file. In the future, we can think about having this file be data output, and if necessary, load it and a saved version and check for congruence.

Yeah, tests look totally 馃憣 to me!

This PR mostly fixes #22, but doesn't include the last component, which is the matrix expansion of defaults. I suggest we postpone that, and make it a new issue.

Yeppers, sounds good.

Copy link
Member

@elbeejay elbeejay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool idea Eric, I like it.

@amoodie pointed out some issues with the PR. changed some importing 
code, and one order of operations problem.
@ericbarefoot
Copy link
Collaborator Author

Thanks y'all. I will go ahead and merge this when it passes checks.

Surprise! I didn't know how this worked. Turns out you don't need to 
import the whole package anyway. derp derp
@ericbarefoot ericbarefoot merged commit d089909 into DeltaRCM:develop May 19, 2020
@ericbarefoot ericbarefoot deleted the iss22 branch May 19, 2020 20:19
@amoodie
Copy link
Member

amoodie commented May 19, 2020

awesome, thanks @ericbarefoot

馃帀 10/10 PR experience, would review again 馃帀

@ericbarefoot ericbarefoot mentioned this pull request Jun 2, 2020
amoodie pushed a commit to amoodie/pyDeltaRCM that referenced this pull request Feb 25, 2021
Adding command line interface to model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reworking the yaml input configuration
3 participants