Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 basic
pyproject.toml
for installing dependencies #13Add basic
pyproject.toml
for installing dependencies #13Changes from all commits
ada433f
1f58747
c0c31d6
1d980b6
857a92d
049d69f
f9979f3
6f22e36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that stuff gets stored here after data is downloaded.
We will almost certainly change this when tackling #8 but is it worth setting to a specified local location for now?
Given the data seems to be stored after this script is run I'd also question whether
tempfile.mkdtemp()
is the correct command to use given the docs seem to imply it is for temp dirs that should be closed and removed before completion.This probably isn't the place to deal with this, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, MLflow handles saving output:
Which currently goes to
./mlruns/0/<hash>/artifacts/forcing
. I think this is a tempdir and used correctly. Still useful to enable overriding (it was done so here), but I have a feelingtempfile
can read envvars to do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used correctly or incorrectly?
I didn't like the use of a tempfile to save important data when I saw it, and really tmpfiles should be cleared before ending the script.
I think you can probably just commit this as it should be dealt with in #4 and doesn't really matter where it creates it for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably write something tentative here, and/or as Arthur to provide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are imposing black we can add some of this into the file as follows:
This is optional though and I'm not sure I fully appreciate what it does at the moment.
Similar tools exist for linting etc.