Added option to save to full paths #45
Conversation
menamerai
force-pushed
the
42-static-dir-follow-ups
branch
from
d0991a3 to
4e490ac
Compare
scripts/label_all_tokens.py
Outdated
| else: | ||
| filepath = os.path.expandvars(args.output) | ||
| print(f"Outputting to path {filepath}") | ||
|
|
||
| with open(f"{filepath}", "wb") as f: |
There was a problem hiding this comment.
| with open(f"{filepath}", "wb") as f: | |
| with open(filepath, "wb") as f: |
scripts/label_all_tokens.py
Outdated
| filepath = STATIC_ASSETS_DIR.joinpath(args.output) | ||
| print(f"Outputting file {args.output} to path {filepath}") | ||
| else: | ||
| filepath = os.path.expandvars(args.output) |
There was a problem hiding this comment.
Oh, sorry, think I misinterpreted this - you're asking "why" about the whole block, in which case the answer is "because that's the straightforward reading of the issue that I wrote". I should have just made the issue "use the path the user gives without any reference to the static dir" - so that's on me.
There was a problem hiding this comment.
No, I was asking just about expandvars - I haven't seen this before
scripts/label_all_tokens.py
Outdated
| @@ -32,10 +33,14 @@ def main(): | |||
| default="delphi-suite/delphi-llama2-100k", | |||
| required=False, | |||
| ) | |||
| parser.add_argument( | |||
| "--output", | |||
| help="Output path name. Must include at least output file name.", | |||
There was a problem hiding this comment.
I think it's either output path or output file name, not output path name. This is a bit weird though, because: what if I want to save in my current working directory? I'll just pass "myfilename.pkl" meaning "./myfilename.pkl" but it'll save in static dir.
There was a problem hiding this comment.
I endorse both points.
On reflection, we should be writing to the static dir very rarely, and when we do it's reasonable to expect the user to move the files there and check them in. The main purpose of the static dir is to have a straightforward way to read.
So output paths should just use the path the user passes in the most straightforward way possible, without any reference to the static dir.
There was a problem hiding this comment.
Earlier in the code, line 60-69, there is a section that writes down all_tokens_list.txt, which the script isn't giving the user a choice on where to load in the script argument. Should I leave the code as-is, letting the script saving this to static or give it the same treatment as the labelled token pickle by adding an extra argument?
There was a problem hiding this comment.
Looks broadly good. I agree with @jettjaniak's comments. I'm sorry about the extra hassle, which is my fault - I should have just written "cut references to any particular dir" as part of the issue.
menamerai
force-pushed
the
42-static-dir-follow-ups
branch
from
d447d11 to
20843c5
Compare
|
Hey, I would not endorse using the Also, the first message of this PR states that All in all I am not sure, if this PR will now fix any issue not part of #40 ? :/ |
|
Hey @joshuawe. Yes Also, yes I agree that a command line argument to choose the save path would be great, but if it is already included in #40, then I don't think that this PR is very productive. @jaidhyani do you think I should close this PR, or is there something different you had in mind that I could work towards? |
Address issue #42
Added options to script to allow users to save to full path instead of the fixed static dir. Also modified the
scripts/map_tokens.pyfile for consistency in scripts.I noticed that the part to allow for this save option is similar across scripts. Should I make a general function for this?