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

Prediction runs both locally and via S3 #236

Merged
merged 9 commits into from
Feb 5, 2024
Merged

Conversation

ultiwinter
Copy link
Contributor

@ultiwinter ultiwinter commented Feb 4, 2024

Now Merchant Size Prediction works both locally and via S3

  • Adjusted the paths in each case of local or S3 prediction.
  • Files are saved accordingly.
  • Models can be imported both locally and via S3

Note: Due to shortage of time, please feel free to modify right away!

@ultiwinter ultiwinter self-assigned this Feb 4, 2024
@ultiwinter ultiwinter added the bugfix Something isn't working label Feb 4, 2024
@ultiwinter ultiwinter linked an issue Feb 4, 2024 that may be closed by this pull request
@ultiwinter ultiwinter changed the title Prediction runs both locally and on S3 Prediction runs both locally and via S3 Feb 4, 2024
Copy link
Contributor

@Tims777 Tims777 left a comment

Choose a reason for hiding this comment

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

Looks good so far, I will try it out tomorrow. Once that is done, I will approve the PR👍

One thing I noticed is that you use if S3_bool: / else: quite often. I think it would be cleaner to have that logic in the database abstraction layer instead. However, due to the time constraints and because it will not impact our ability to demo the software in any way, I would suggest to just leave it like that.

src/demo/demos.py Outdated Show resolved Hide resolved
src/demo/demos.py Show resolved Hide resolved
Signed-off-by: Ahmed Sheta <[email protected]>
Copy link
Collaborator

@luccalb luccalb left a comment

Choose a reason for hiding this comment

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

Just a question for understanding why we don't use the 100k file for prediction? Is it because the leads/enriched.csv didnt have merchant sizes before?

src/demo/demos.py Show resolved Hide resolved
src/demo/demos.py Show resolved Hide resolved
src/preprocessing/preprocessing.py Show resolved Hide resolved
@luccalb
Copy link
Collaborator

luccalb commented Feb 5, 2024

I've added some minor changes to improve the stability of running the merchant size prediciton locally

Copy link
Collaborator

@luccalb luccalb left a comment

Choose a reason for hiding this comment

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

Fine by me

@ultiwinter ultiwinter merged commit d842143 into dev Feb 5, 2024
6 checks passed
@ultiwinter ultiwinter deleted the bugfix/datanames-ahmed branch February 5, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Something isn't working
Projects
None yet
4 participants