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

Python backend #1

Closed
wants to merge 14 commits into from
Closed

Python backend #1

wants to merge 14 commits into from

Conversation

fdegier
Copy link

@fdegier fdegier commented Oct 20, 2022

Hi @thakkarparth007 👋

I like your PR a lot, yesterday I introduced quite a few changes so I helped you resolve the conflicts. I also ran into a few issues with the setup script which I fixed, can you do some testing? In fauxpilot#86 I've left additional comments.

@thakkarparth007
Copy link
Owner

Hi @fdegier thanks for the help! I'll test the changes and get back over the weekend.

with open(os.path.join(model_dir_path, '../config.pbtxt'), 'w') as f:
f.write(config)
print(f"Config written to")
Copy link
Owner

Choose a reason for hiding this comment

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

Minor: Seems incomplete

Copy link
Author

Choose a reason for hiding this comment

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

The python script was running so fast, I thought it was not being called so I added a print statement and indeed its incomplete. Would it make sense to add a print statement here to notify users that it ran successfully?

setup.sh Outdated

# Write .env
echo "NUM_GPUS=${NUM_GPUS}" >> .env
echo "MODEL_DIR=${MODEL_DIR}/${MODEL}-${NUM_GPUS}gpu" >> .env
Copy link
Owner

Choose a reason for hiding this comment

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

MODEL is initialized later, which makes this line output "MODEL_DIR=.../models/-1gpu".

I think the config should be written in a separate function because the model dir format is different depending upon the backend.

Copy link
Author

Choose a reason for hiding this comment

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

Good call, I've fixed this.

setup.sh Outdated Show resolved Hide resolved
@thakkarparth007
Copy link
Owner

Hi @fdegier I noticed some issues with setup.py while testing the changes. It's mostly related to adding stuff to the .env file. I'll take a stab at the changes and might ask you to test them out if possible

@fdegier
Copy link
Author

fdegier commented Oct 20, 2022

Interesting, for me all manual test cases worked. Which OS are you on? I will be online again on Monday.

@thakkarparth007
Copy link
Owner

thakkarparth007 commented Oct 20, 2022

I'm using Linux via WSL.

image

If you see here, MODEL isn't initialized here (it's initialized inside the backend functions). As a result, MODEL_DIR is not set correctly. This issue didn't arise in your original PR to moyix's repo because MODEL was initialized over there:

image

Weirdly, I'm unable to locate the commit where the MODEL assignment was removed but I'm guessing it got lost in the merge conflict resolution.

@fdegier fdegier closed this by deleting the head repository Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants