Skip to content

Have different cache directories for different servers - #432

Merged
mfeurer merged 15 commits into
developfrom
fix/#397
Apr 11, 2018
Merged

Have different cache directories for different servers#432
mfeurer merged 15 commits into
developfrom
fix/#397

Conversation

@mfeurer

Copy link
Copy Markdown
Collaborator

No description provided.

@mfeurer
mfeurer requested a review from janvanrijnApril 4, 2018 10:02
@mfeurermfeurer mentioned this pull request Apr 4, 2018

@janvanrijnjanvanrijn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Up to discretion of @mfeurer

  • we don't need to create cache directory structure already in set_cache_dir() fn
  • for this reason the config object doesn't need getters and setters

I will approve once the unit tests succeed

@mfeurer

Copy link
Copy Markdown
CollaboratorAuthor

This now also partially solves #396. However, not as nicely as expected.

@janvanrijnjanvanrijn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved. But Please address my comments :)

def test_get_setup(self):
# no setups in default test server
openml.config.server = 'https://www.openml.org/api/v1/xml/'
openml.config.set_server_url('https://www.openml.org/api/v1/xml/')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is this still a fn?

Comment threadopenml/testing.py Outdated
def tearDown(self):
os.chdir(self.cwd)
shutil.rmtree(self.workdir)
for i in range(10):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

make 10 a variable?

@ArlindKadra

ArlindKadra commented Apr 10, 2018

Copy link
Copy Markdown
Member

@mfeurer@janvanrijn If you like the current implementation, you can merge the pull the request. Build is passing.

@mfeurer

Copy link
Copy Markdown
CollaboratorAuthor

Thanks for the fix @ArlindKadra. Could you please describe which change was actually necessary to fix it? Also, could you please explain the reasoning for changing the static_cache_dir variable?

@mfeurer
mfeurer merged commit 87ad7b1 into developApr 11, 2018
@mfeurer
mfeurer deleted the fix/#397 branch April 11, 2018 08:30
Sign up for freeto 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.

3 participants

@mfeurer@ArlindKadra@janvanrijn