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

bugfix for training within git repo #195

Merged
merged 1 commit into from
Aug 4, 2021
Merged

bugfix for training within git repo #195

merged 1 commit into from
Aug 4, 2021

Conversation

hesther
Copy link
Contributor

@hesther hesther commented Aug 3, 2021

Currently, tap causes an issue when training a model within a git repo that has no origin specified. To reproduce the error:
mkdir test; cd test; git init
python <path_to_chemprop>/train.py --data_path <path_to_data_csv> --dataset_type regression
This is a tap issue since when writing arguments to file, it runs git remote get-url origin, in the current folder, not in the chemprop folder (which would be correct). I will open a tap issue too, but to stay compatible with older versions of tap (and until they fix it), I think it would be good to have a workaround.

@hesther
Copy link
Contributor Author

hesther commented Aug 3, 2021

I have also reported this to Kyle: TAP Issue 57

Copy link
Contributor

@cjmcgill cjmcgill left a comment

Choose a reason for hiding this comment

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

Tested it and it works. Looks good!

@hesther
Copy link
Contributor Author

hesther commented Aug 4, 2021

Should I just merge it then?

@cjmcgill
Copy link
Contributor

cjmcgill commented Aug 4, 2021

I wasn’t sure if it made more sense for me to go ahead and merge it or if I needed to let the decision making initiative go back to you one more time. I’m not sure what the best practice is for the situation where it’s the two of us reviewing each other. But I guess maybe if no further delay is necessary then it makes no sense to create an extra step. I think I was overthinking this.

@cjmcgill cjmcgill merged commit 3f20ca4 into master Aug 4, 2021
@cjmcgill cjmcgill deleted the bugfix_tap branch August 4, 2021 05:19
@hesther
Copy link
Contributor Author

hesther commented Aug 4, 2021

Great! Yeah, I wasn't sure whether we wanted to merge PR 190 and 193 first.

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.

None yet

2 participants