fidelity / spock

spock is a framework that helps manage complex parameter configurations during research and development of Python applications
https://fidelity.github.io/spock/
Apache License 2.0
122 stars 13 forks source link

SavePath from config.yaml file #241

Closed svenstehle closed 2 years ago

svenstehle commented 2 years ago

Hi :) Spock is cool and I like what I see so far. I appreciate the work you put into Spock and after using Hydra for over a year I want to check out Spock for configuration. However, I am having some troubles with it.

Describe the bug Cannot load in a SavePath for the spock configuration used during a run from the config.yaml provided. Right now my ugly workaround is to hardcode the path with save_path = f"{environ.get('PWD')}/runs" and SpockBuilder.save(user_specified_path=save_path)

To Reproduce Steps to reproduce the behavior:

  1. instantiate spock namespace with SpockBuilder(somecfg, desc="somecfg", config=['src/config.yaml']).save(file_extension=".toml").generate()`
  2. ValueError Exception was raised: ValueError: Save did not receive a valid path from: (1) markup file(s) or (2) the keyword arg user_specified_path

Expected behavior I want to specify a mutable path for outputs/saving in the external config.yaml file. Having to do this hardcoded in user_specified_path inside the main file is an inferior alternative.

Desktop (please complete the following information):

Additional context I hope I am missing it in the docs, but I cannot find it here: SavePath Here it says We simply specify a SavePath in a spock config, which is a special argument type that is used to set the save path from a configuration file., which does not work for me. I also do not know where to import this SavePath type from.

Thanks in advance for your help, Sven

ncilfone commented 2 years ago

Hey @svenstehle!

Thanks for the kind words. Hydra is a pretty great library too (it didn't exist when we first wrote Spock internally) but hopefully both fit into their own respectively different niches.

I think maybe the docs were not clear enough from my end... There are two ways to save in Spock: (1) via the SavePath type which can be imported from the top level of spock or (2) via the user_specified_path kwarg within the save function. (Note that user_specified_path will take precedence over SavePath)

Option 1: SavePath Example

# -*- coding: utf-8 -*-
from spock import SpockBuilder
from spock import spock
from spock import SavePath

@spock
class Simple:
    only: int = 1
    save_path: SavePath

def main():
    test = SpockBuilder(
        Simple, 
        lazy=True, 
        configs=["/path/to/config/issue.toml"]
    ).save(file_extension=".toml").generate()
    print(test)

if __name__ == '__main__':
    main()

With the config file as:

[Simple]
    save_path = "/tmp"

Option 2: Specify Path In-Line

# -*- coding: utf-8 -*-
from spock import SpockBuilder
from spock import spock

@spock
class Simple:
    only: int = 1

def main():
    test = SpockBuilder(
        Simple, 
        lazy=True, 
        configs=["/path/to/config/issue.toml"]
    ).save(file_extension=".toml", user_specified_path='/tmp').generate()
    print(test)

if __name__ == '__main__':
    main()

Both of those should be working examples that you should be able to run locally (with correct paths for your system).

I'm updating the docs to be a bit clearer that these are the two ways to specify save paths and to indicate that SavePath can be imported from the top level.

Hope this helps clear up the confusion!

Best,

Nick

svenstehle commented 2 years ago

Hi Nick, thanks for the very fast response and the great working examples :)

They both work on my end, thanks! On a side-note: Hydra may be newer, but Spock already fixes the testability issue I have with Hydra with the no_cmd_line=True in the SpockBuilder when using something like pytest --pdb tests. I think that is a very necessary thing to work around conflicts of that nature.

I have a few follow-up questions, though. Please tell if I should open/move this to another issue or feature request :)

I have played around some more with Spock and found from spock.backend.typed import SavePath, which lets VSCode know the type of this parameter and surprisingly also works functionally, i.e. I can adjust the save_path in the config and Spock will save the config in the correct adjusted save_path without specifying user_specified_path. Nice! But is this intended? When I use from spock import SavePath, VSCode does not recognize the type of SavePath and leaves it as Any

On another matter, and maybe you will tell me I should open a feature request for this, I have learned to use OmegaConf while working with Hydra, and I would love to have this possibility of interpolation and accessing environment variables and other resolvers inside the config.yaml, e.g.

# config.yaml
save_path: ${oc.env:PWD}/runs

This gives so much flexibility. Are there plans to integrate OmegaConf into Spock, would this even be feasible?

ncilfone commented 2 years ago

I have played around some more with Spock and found from spock.backend.typed import SavePath, which lets VSCode know the type of this parameter and surprisingly also works functionally, i.e. I can adjust the save_path in the config and Spock will save the config in the correct adjusted save_path without specifying user_specified_path. Nice! But is this intended? When I use from spock import SavePath, VSCode does not recognize the type of SavePath and leaves it as Any

Ahhh this makes sense. There are some stubs in various places to get type hinting and autocompletion to work in VSCode when using spock objects (see here). Should be a simple fix to the stub to make sure the type hinting works from the top level import.. I'll get a PR in to change this, no need for a new issue as this should be a very minor change

On another matter, and maybe you will tell me I should open a feature request for this, I have learned to use OmegaConf while working with Hydra, and I would love to have this possibility of interpolation and accessing environment variables and other resolvers inside the config.yaml,

Please do open a feature request for this. We can't support OmegaConf directly (as it provides a lot of similar functionality that we already leverage from attrs as an underlying library), but we should definitely be able to replicate this functionality. Shouldn't be too much legwork on my end. I'll add it to my todo list. Happy to have you help/contribute if you want as well :-)

Nick

svenstehle commented 2 years ago

Thanks for taking this on! :)

Please do open a feature request for this. We can't support OmegaConf directly (as it provides a lot of similar functionality >that we already leverage from attrs as an underlying library), but we should definitely be able to replicate this functionality. >Shouldn't be too much legwork on my end. I'll add it to my todo list. Happy to have you help/contribute if you want as well >:-)

I will open a feature request for this then. I am happy to help/contribute on this. I would not know where to begin with this, however.

ncilfone commented 2 years ago

closing per resolution and new open feature request