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

Allow remote schemas. Fix #160 #162

Merged
merged 6 commits into from
Apr 2, 2024
Merged

Allow remote schemas. Fix #160 #162

merged 6 commits into from
Apr 2, 2024

Conversation

nsheff
Copy link
Contributor

@nsheff nsheff commented Feb 22, 2024

This allows the schemas to be remote. I did this by using yacman.load_yaml, instead of an internal read_yaml_data which did not handle URLs. I also replaced an internal mk_abs_via_cfg function call with ubiquerg.mkabs --

  • Should we replace all mk_abs_via_cfg use by mkabs, and then remove that function?
  • Should we replace all the calls to the internal read_yaml_datawith the yacman.load_yaml? This would require a bit of refactoring because the internal read_yaml_data was not just reading yaml, but was used for a few other things as well. In general I don't like that entanglement, and it would reduce maintenance to just use yacman.load_yaml.
  • update documentation to make use of this nicety to explain things easier. (for example, here: https://pep.databio.org/pipestat/code/python-tutorial/)

One nice thing about this is that it will make it so much easier to get started using pipestat. Our tutorials can just gloss over the schema complexity and point to some general-purposes, training schemas, so then the user doesn't have to create one locally to just get started quickly.

TODO:

@donaldcampbelljr
Copy link
Contributor

Do you have an ETA on ubiquerg 0.8.0? Sometime in the next week?

@nsheff
Copy link
Contributor Author

nsheff commented Feb 22, 2024

can be anytime. you can release it if it helps you. I don't have anything immediately planned to add to it.

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 95.12195% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 55.12%. Comparing base (4daf0c6) to head (a091ecb).
Report is 6 commits behind head on dev.

❗ Current head a091ecb differs from pull request most recent head 3e00e41. Consider uploading reports for the commit 3e00e41 to get more accurate results

Files Patch % Lines
pipestat/parsed_schema.py 94.11% 1 Missing ⚠️
pipestat/pipestat.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #162      +/-   ##
==========================================
+ Coverage   54.50%   55.12%   +0.61%     
==========================================
  Files          15       15              
  Lines        2341     2324      -17     
==========================================
+ Hits         1276     1281       +5     
+ Misses       1065     1043      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@donaldcampbelljr
Copy link
Contributor

mk_abs_via_cfg contains the additional step of using os.makedirs to create the full path if it does not already exist. This is important for the file backend where the user could specify a results file that is nested into subfolders that don't exist yet. I added a commit that moved this functionality into a small separate func so that mkabs can be used everywhere and then we can create the subdirectories to the file path only when absolutely needed.

@donaldcampbelljr
Copy link
Contributor

I've also refactored read_yaml_data with yacman.load_yaml.

I will go ahead and merge these changes to dev, adding the action item to update the documentation to the original issue in #160

@donaldcampbelljr donaldcampbelljr marked this pull request as ready for review April 2, 2024 23:13
@donaldcampbelljr donaldcampbelljr merged commit 33c8d92 into dev Apr 2, 2024
3 checks passed
@donaldcampbelljr donaldcampbelljr deleted the remote-schemas branch April 2, 2024 23:21
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