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

Add script to generate config file #520

Closed

Conversation

bfhealy
Copy link
Collaborator

@bfhealy bfhealy commented Dec 14, 2023

This PR adds a script to generate config.defaults.yaml (and config.yaml if it doesn't exist) in support of efforts to make scope pip-installable. The GH Actions workflows are updated to use the new code.

The downside of this change is that we lose comments in the yaml file that could help new users. For this reason, config.yaml.defaults remains unchanged in this repo. (check_configs in scope.py passes with both versions of the default file). We should also augment the setup documentation in this regard once a pip-installable scope package is realized.

@bfhealy bfhealy temporarily deployed to Integrate Pull Request December 14, 2023 19:49 — with GitHub Actions Inactive
@bfhealy bfhealy temporarily deployed to Integrate Pull Request December 14, 2023 19:49 — with GitHub Actions Inactive
Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

Maybe create_config should read from the existing templates file somehow? Or it could live in some kind of data folder that is packaged in pypi?

@bfhealy
Copy link
Collaborator Author

bfhealy commented Dec 14, 2023

I'd be in favor of those approaches. I'll look into including the default file in the installed package.

@bfhealy
Copy link
Collaborator Author

bfhealy commented Dec 14, 2023

@mcoughlin I'm able to get config.yaml.defaults to download when running pip install scope from test-pypi. The only issue is that the file appears in my environment's site-packages path (/Users/bhealy/miniforge3/envs/scope-env-poetry/lib/python3.10/site-packages), and users will similarly have to navigate to site-packages to set up their own config file. This seems fine for config setup, but it's not ideal for all of scope's outputs to be saved within this directory.

I'm thinking I can add a script to create a basic set of scope directories in the current working directory, and then allow that base directory to be specified in the config file. This will require changes to the code wherever we currently use __file__ to define the base directory, but it will decouple all scope activities from site-packages except the configuration.

@mcoughlin
Copy link
Collaborator

@bfhealy All that makes sense. Might also check with Theo if he has thoughts too.

@bfhealy
Copy link
Collaborator Author

bfhealy commented Dec 14, 2023

Hi @Theodlz - we're considering how best to package scope for a PyPI release and have been discussing issues about the config file, where it gets installed, and how users interact with scope (see above). Do you have any thoughts on these issues and how best to address them?

@Theodlz
Copy link
Collaborator

Theodlz commented Dec 14, 2023

Hi @Theodlz - we're considering how best to package scope for a PyPI release and have been discussing issues about the config file, where it gets installed, and how users interact with scope (see above). Do you have any thoughts on these issues and how best to address them?

Hi Brian! Yes, a few suggestions, though I see that some of your changes go in that direction as well:

  • Have a config.yaml.defaults file live somewhere in the code, either as a proper file or a dictionary used to generate it.
  • When using the scope package, require your users to pass a config file as an argument when they call their code, or at least ask for a config (as dict or Config object) to be passed to your ScoPe class or methods. It could be a method from scope that takes a config as an input or a path on disk and loads it in a way that thereafter it's accessible anytime when the code is running.
  • If you do generate files, as you mentioned you'll want to generate them in the current directory. However what if the user wants the config to live somewhere else? You need to have it passed as an argument somehow if desired.
  • I'm not sure how you want to handle config(s) once loaded in Python, but I would copy/paste the config class found in the nmma-api repo. It's an extension of the SkyPortal/baselayer one, that can take environment variables as well instead of a file and generate the config with that. It's a cool way to also grab config variables, which make everyone's life easier when deploying on some cloud services that don't allow you easily to pass a file that's not on the git repo directly.

Overall I would go for a system where you can specify a config yaml if you'd like. Or maybe, you'd rather want the user to pass the config in the code, and it's their job to handle configuration. Maybe you can just provide the config class to load a config yaml, a method to generate a config yaml given a location, but then let them take care of whether or not they want to use that or just pass the config the way they want it to scope. Does that make sense? I'm afraid that forcing them to use a specific way to pass the config other than in the code itself, explicitly, might actually turn out to be a limiting factor in some applications.

If you'd like, I'm happy to open a PR this weekend with some suggestions which you can keep or not in the final version

@bfhealy
Copy link
Collaborator Author

bfhealy commented Dec 14, 2023

These are very helpful points, thanks Theo! I'd welcome a PR with some suggestions as well. I definitely want to avoid implementing anything in the release that becomes a limitation for new users.

@bfhealy
Copy link
Collaborator Author

bfhealy commented Feb 22, 2024

No longer needed due to changes in #514.

@bfhealy bfhealy closed this Feb 22, 2024
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.

3 participants