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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change extra_providers to load scripts #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Zaharid
Copy link
Contributor

@Zaharid Zaharid commented Nov 8, 2017

Now the extra providers are loaded from a file path instead of an
import specification making them easier to use for causal tests.

There are a few open questions:

  • Whether we want to also have the old functionality of loading
    actual modules
  • How do the error messages look like when something fails in the
    imported provider?
  • Do we add a way to specify extra providers (module) in the config
    file? These are geared toward supporting something like
    validphys.applgrid where we do not commit to supporting some
    crazy setup by default.

cc @nhartland

Now the extra providers are loaded from a file path instead of an
import specification making them easier to use for causal tests.

There are a few open questions:

  - Whether we want to also have the old functionality of loading
    actual modules
  - How do the error messages look like when something fails in the
    imported provider?
  - Do we add a way to specify extra providers (module) in the config
    file? These are geared toward supporting something like
    `validphys.applgrid` where we do not commit to supporting some
    crazy setup by default.
@nhartland
Copy link
Member

Whether we want to also have the old functionality of loading
actual modules

You mean essentially loading everything as "Extra modules"?
I guess it doesn't make much sense to encourage two different loading mechanisms,
so going with the more flexible option makes sense.

Do we add a way to specify extra providers (module) in the config
file? These are geared toward supporting something like
validphys.applgrid where we do not commit to supporting some
crazy setup by default.

Makes sense to me, although I suppose there is an argument that we'd generally want to discourage this sort of option unless it's really necessary. So maybe it shouldn't be too easy!

@Zaharid
Copy link
Contributor Author

Zaharid commented Nov 11, 2017

After reflecting further, I think that if we only allow one way of passing extra scripts, then it should be the old (i.e. current) one, possibly with the UI changed so it is enabled in the config file.

I really like to have the invaraint that valipdhys <output>/input/rucard.yamlreproduces the result of the output for any runcard, and that everything that is needed is either downloaded or included in the input folder.
Having a module based loading in principle allows to store scripts in a canonical location so other people can put the script somewhere (i.e. install) and have that working somewhat automatically. The use case for quick hacks becomes a little more difficult, since you have to mess with symlinks or PYTHONPATH or setuptools, but that should not be used for anything beyond personal tests, since by definition it's not reproducible.

The other use cases, namely having non standard dependencies certainly are favoured by the package based approach.

Regarding having the option in config, it surely allows for arbitrary code execution from reportengine, which may not be all that good for security.

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

Successfully merging this pull request may close these issues.

2 participants