Replace the restriction to use Repository inside step with a warning #792
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
bot
added
internal
stefannica
force-pushed
the
bug/allow-repo-access
branch
from
5340bd4 to
f723e71
Compare
fa9r
left a comment
fa9r
left a comment
There was a problem hiding this comment.
Love the change! Only major question I have: are there some methods in Repository() that really should not be used from within a step and might lead to weird behavior?
| "@step(enable_cache=False)\n", | ||
| "def prediction_service_loader() -> BaseService:\n", | ||
| " \"\"\"Load the model service of our train_evaluate_deploy_pipeline.\"\"\"\n", | ||
| " repo = Repository(skip_repository_check=True)\n", |
There was a problem hiding this comment.
why was this removed? Isn't it still nicer to suppress those warning when running the quickstart?
There was a problem hiding this comment.
same for the other change below
There was a problem hiding this comment.
you had a valid point here. I didn't want to expose users to this hidden variable, so I removed it from here. But the correct way to hide it is to detect whether a step has caching enabled from within the step and only show that warning if that is the case. Which is what I did in the last commit.
stefannica
force-pushed
the
bug/allow-repo-access
branch
from
f723e71 to
875c51b
Compare
stefannica
force-pushed
the
bug/allow-repo-access
branch
from
875c51b to
47e258f
Compare
Describe changes
The restriction to use Repository inside step is an antiquated measure that has become a nuisance. This PR replaces it with a warning and removes some other unneeded code designed to address this same problem.
Pre-requisites
Please ensure you have done the following:
Types of changes