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

Mentos integration #54

Merged
merged 29 commits into from
Mar 10, 2017
Merged

Mentos integration #54

merged 29 commits into from
Mar 10, 2017

Conversation

Arttii
Copy link

@Arttii Arttii commented Feb 6, 2017

Description

Migrating satyr to new CI, Naming and underlying driver to Mentos and Daskos

Motivation and Context

  • A pure python high level implementation
  • One general solution for all related packages

How Has This Been Tested?

Tests are still pending

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Review:

@kszucs

  • First assignee approved
  • Second assignee approved

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

So far so good, just some comments. I know its under construction.

return codecs.unicode_escape_decode(string)[0]

# dict.iteritems(), dict.iterkeys() is also incompatible
if six.PY3:
Copy link
Member

Choose a reason for hiding this comment

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

iteritems and iterkeys are importable from six

from six import iteritems, iterkeys

Copy link
Author

Choose a reason for hiding this comment

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

Ya I know, I stole that block of code and was lazy

except KeyError:
raise AttributeError(k)
else:
object.__delattr__(self, k)
Copy link
Member

Choose a reason for hiding this comment

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

Are these for dot notation?

Copy link
Author

Choose a reason for hiding this comment

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

Yes

return Message({'name': 'disk', 'role': '*', 'scalar': Message({'value':value}), 'type': 'SCALAR'})

def Ports(begin,end):
return Message({'name': 'disk', 'role': '*', 'ranges': Message({'range':[Message({'begin': begin, 'end': end})]}), 'type': 'SCALAR'})
Copy link
Member

Choose a reason for hiding this comment

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

Needs formatting

Copy link
Author

Choose a reason for hiding this comment

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

God yes, maybe setup autopep8 or something, I did not do anything on this package

b = self.flatten(other)
out = {}
for k, v in self.zeroes.items():
if k=="ports":
Copy link
Member

Choose a reason for hiding this comment

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

With supporting ports it could be cleaner to create scalar and range types and convert the resources when initializing the ResourceMixin.

return self.keys()

def Cpus(value):
return Message({'name': 'cpus', 'role': '*', 'scalar': Message({'value':value}), 'type': 'SCALAR'})
Copy link
Member

Choose a reason for hiding this comment

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

Handling role will be an interesting question.


def __init__(self, docker='satyr', force_pull=False,
envs={}, uris=[], **kwds):
super(PythonExecutor, self).__init__(**kwds)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use something like a template with pure dict notation instead of Message constructors.

Copy link
Author

Choose a reason for hiding this comment

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

So like the Cpus and Disk?

@@ -21,10 +21,8 @@ def weight(items, **kwargs):

def ff(items, targets):
"""First-Fit

Copy link
Member

Choose a reason for hiding this comment

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

What is your problem with my whitespaces? :)

Copy link
Author

Choose a reason for hiding this comment

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

It be my editor I think :D

mentor/queues.py Outdated
self.client = client
def __reduce__(self):
hosts = ",".join(["{}:{}".format(h, p) for h, p in self.client.hosts])
return (SerializableMixin.init, (self.__class__, hosts, self.path))
Copy link
Member

Choose a reason for hiding this comment

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

Cleaner indeed.

@@ -1,28 +0,0 @@
- repo: https://github.com/pre-commit/pre-commit-hooks.git
Copy link
Member

Choose a reason for hiding this comment

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

I like to use these hooks http://pre-commit.com/

Enforce me to keep the conventions. Do You have alternatives?

@kszucs
Copy link
Member

kszucs commented Feb 7, 2017

Nice to see the process :)

@kszucs
Copy link
Member

kszucs commented Feb 24, 2017

What is the status of this? I'd like to implement a framework for distributed, but kinda blocked this PR.

@Arttii
Copy link
Author

Arttii commented Feb 28, 2017

Well, it needs a fairly extended rewrite I think or rethink of how we do things. I did not have time to work on this recently as well. Need to properly understand how to do it.

@kszucs
Copy link
Member

kszucs commented Feb 28, 2017

Let me know if I can help.

@Arttii
Copy link
Author

Arttii commented Feb 28, 2017

What do you think are the main necessities we need in mentor?

Because stuff like retry and etc are already handled in mentos, and the way it works with tornado callbacks we cannot intercept exceptions, but have to kinda propagate errors as events, meaning the function evaluation thing has to be fully redone. I am also very unsure about the function execution interface.

You can take a look at the currently state. Its's a mess i think.

@kszucs
Copy link
Member

kszucs commented Feb 28, 2017

Is the like retry configurable in mentos? Can You link the source of the retry logic?

I am also very unsure about the function execution interface.

Do You have proposals?

@Arttii
Copy link
Author

Arttii commented Feb 28, 2017

The retry is not directly exposed to the schedulers as of now, but can be made a global config I guess.

I am quite unsure to be honest. I keep getting caught up in making everything work with the existing codebase.

@kszucs
Copy link
Member

kszucs commented Feb 28, 2017

Ohh, this retry logic is great. I didn't know that mentos has it.

The problem is that we have multiple options to place the scheduling logic: scheduler, executor, even task. We also need to distinguish between homogeneous and heterogeneous tasks.

We should specify a couple of frameworks (on pseudo code level) to plan the proper interface for mentor. We might need multiple interfaces.

@kszucs
Copy link
Member

kszucs commented Feb 28, 2017

I've created a new issue #55 to plan the interface.

@kszucs kszucs changed the base branch from master to mentos March 10, 2017 07:39
@kszucs
Copy link
Member

kszucs commented Mar 10, 2017

I'm closing this PR now, created a new mentos branch with your modifications.

@kszucs kszucs closed this Mar 10, 2017
@kszucs kszucs reopened this Mar 10, 2017
@kszucs kszucs merged commit 362d38c into daskos:mentos Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants