Status quo caching extension

[tfesenbecker]

Current status of the caching extension of lapis.
The new code contains

• Storage class: describes storage objects like caches and has the basic functionalities to place new files in the cache or update them when
• FileProvider class: objects handle communication between jobs and storage objects, currently very basic, could take limited bandwith into consideration
• readout for storage object input files
• drones have a new attribute "sitename" that can be used to select storage elements that could be accessed by jobs running inside a drone

As this is only a draft the code contains different debug outputs to track the execution of jobs.

[tfesenbecker]

I'll have a look at the UnitTests

[codecov]

Codecov Report

Merging #53 into master will decrease coverage by 1.9%.
The diff coverage is 18.05%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   37.34%   35.43%   -1.91%     
==========================================
  Files          15       18       +3     
  Lines         739      807      +68     
  Branches      110      122      +12     
==========================================
+ Hits          276      286      +10     
- Misses        449      506      +57     
- Partials       14       15       +1

Impacted Files	Coverage Δ
lapis/pool_io/htcondor.py	76.92% <ø> (ø)	⬆️
lapis/storage_io/storage.py	0% <0%> (ø)
lapis/cli/simulate.py	0% <0%> (ø)	⬆️
lapis/file_provider.py	0% <0%> (ø)
lapis/simulator.py	0% <0%> (ø)	⬆️
lapis/drone.py	60.41% <100%> (+0.84%)	⬆️
lapis/scheduler.py	15.68% <40%> (-0.48%)	⬇️
lapis/utilities/walltime_models.py	66.66% <66.66%> (ø)
lapis/job.py	73.91% <77.77%> (+0.14%)	⬆️
lapis/job_io/swf.py	90.9% <0%> (ø)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a698820...32faa38. Read the comment docs.

[pep8speaks]

Hello @tfesenbecker! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-08 09:14:17 UTC

[eileen-kuehn]

First, great progress! 🥇

General

Please try to attach docstrings to methods and classes. You can also include information on what you are still planning to do, what is not working yet, etc. This helps in reading the new stuff going on here and can improve our feedback for you :)

Comments to Caching Approach itself

It currently seems to me that parts of files can only be added to a storage but never be deleted again.

Importing Storage Information

In general, I am currently not yet convinced to import the storage data as it is done with the other input files. It makes a difference. The other input files are exported from existing tools and have defined formatting. So we define in- and output methods. For the storage it is different, it is for setting up the environment for simulation. So more an in-simulation-configuration.
What I currently also don't know is if we could combine those information with information from configuration data from Tardis (I am planning to also support importing those).

[eileen-kuehn]

Could you please reason about including sitename into the drone? From my point of view, this might be too specific. I currently also don't see the advantages from having something like sitename. But I bet you do have some very good reasons! :)

[maxfischer2781]

I'd be careful about using sitename -- it is a very specific jargon/feature (CMS) and may not properly reflect how we need to model caches. E.g. what we've seen is that for a single "site" there may be different cache/storage requirements -- see Chris' results on benchmarking TSystems. If we need to identify the "site", that is basically the Pool of the drone -- which can already be used as a dict key etc.

Is there some advantage to using a string identifier?

[tfesenbecker]

sitename is just quick workaround for the mapping of pools/drones and storage elements. I want to replace this as soon as I understand what information/means of identification will available once other changes to the overall system (adding more of the jobs ClassAd structure, ...) are done. Passing this information directly to the drone was the option that needed the least changes to existing code.

@maxfischer2781 What do you mean by

which can already be used as a dict key etc.

?

[maxfischer2781]

What do you mean by ...

A Pool instance can be used as a key in a mapping -- e.g. Dict[Pool, Storage] should be possible to map from pool (site) to storage and vice versa.

Which of course isn't of much use to you if you want to define this outside of the simulation, e.g. in a JSON/YAML configuration file...

[maxfischer2781]

I've added a mixture of new proposals, food for though and change requests. Note that if you feel a change may be worthwhile but want to get basic functionality first, just add a ticket to add it later. The label recommendation is likely such a case.

[maxfischer2781]

I'd be careful about using sitename -- it is a very specific jargon/feature (CMS) and may not properly reflect how we need to model caches. E.g. what we've seen is that for a single "site" there may be different cache/storage requirements -- see Chris' results on benchmarking TSystems. If we need to identify the "site", that is basically the Pool of the drone -- which can already be used as a dict key etc.

Is there some advantage to using a string identifier?

[eileen-kuehn]

Last night I thought again about the walltime in the jobs regarding the monitoring. The monitoring itself only happens after a job is finished. So this value should not be dependent on an async execution.

So I must revert one of my previous comments: This value must stay an value that you set after you now what its value will actually be. I think this means that the run method of the job itself needs extension. So you must have the connection to the Pipe there that runs as long as not all data is received. So you should remove the await self.walltime condition. See this as a placeholder as we did not have a method to determine the actual wall time before :)

What do you think?

[tfesenbecker]

I'm a bit confused about your mention of the Pipe but we'll see to that later.
I agree with the rest.

[tfesenbecker]

I decided to change the jobs _walltime attribute in case that a new walltime is calculated. I don't think that this should cause any mishaps or difficulties down the road. Do you agree?

[maxfischer2781]

@eileen-kuehn @tfesenbecker as discussed, I've changed the target branch to feature/caching so that we can merge early.