search

microsoft / mwt-ds

Umbrella repository for projects related to the MWT Decision Service
187 stars 77 forks source link

Users/sharathm/azurebatchcf #132

Closed sharathmalladi closed 5 years ago

sharathmalladi commented 5 years ago

This pull request includes prior work for running counterfactual evaluations on Azure Batch as well as the Important Features work.

marco-rossi29 commented 5 years ago

@sharathmalladi Thank you for all the changes.

I'm going to run some tests on my side to ensure LogDownloader and Experimentation are working correctly and then merge.

In the meantime, I'll think if there is a simple way to avoid the code duplication between LogDownloader and AzureUtil

There are two unresolved points. I'm adding comments on them

sharathmalladi commented 5 years ago

The more I think about it, the more I feel we can actually get away without introducing the report_progress input variable and just use: process_checker = update_progress if '__name__' == '__main__' else None

What do you think?

Since LogDownloader can be called from other scripts (usually is), I did not want to add dependency on main since then it will only work if you call LogDownloader directly from python. I wanted to leave it to the caller to decide whether or not to report progress. So I added this report_progress flag - it defaults to true (store_false) if not specified. That way for all other existing scripts, the behavior is not impacted (like you wanted). However for my purposes, I set this flag to false when called from ExperimentationAzure since it is running in Azure batch in a non-interactive fashion.

sharathmalladi commented 5 years ago

ouln't the default w

Shouln't the default we False?

The default is set to true (since you did not want the experience to change for other callers - and this script was essentially logging every time prior to my changes).

sharathmalladi commented 5 years ago

I actually think that there is a small bug, since you never use args.report_progess We should remove this line, and it will be goodvin my opinion

I do use report_progress. download_container() accepts a parameter for report_progress which is being set inside ExperimentationAzure.py script as argument input to this LogDownloader.