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

Adding linting, various fixes. #116

Merged
merged 27 commits into from
May 28, 2019
Merged

Adding linting, various fixes. #116

merged 27 commits into from
May 28, 2019

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented May 28, 2019

No description provided.

Flamefire and others added 23 commits May 27, 2019 17:34
Otherwise this affects all tests scheduled after this
Checks should be done with "is None"/"is not None"
ImageBase defines get_uri(self, image) which is overriden in subclasses
by get_uri(self) potentially causing trouble.
Solution: Extract to free function
Note: get_uri does actually return a protocol(-like), not a URI
It did contain the protocol not the uri
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch vsoch mentioned this pull request May 28, 2019
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

Since we can't link a format type to a version, I've just removed a version check in inspect, and added a simple check for the "data" attribute.

        # Unify output to singularity 3 format
        if "data" in result:
            result = result['data']

This is what should be Singularity version 3 format, anyway.

@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

@Flamefire I'm pinging you here in case you didn't see the previous link - I'm down with gutting out / doing extensive changes to the recipe parser, and then also addressing what you see as issues here. To move forward, I'd like to first finish up this PR (and address other concerns you had here?) and then:

  • fix up the recipe parser as we discussed
  • any other design issues with the main client
  • change testing to pytest
  • updates to other tests and more linting

@Flamefire
Copy link
Contributor

As all my commits are included here and CI passes I'm ok with merging this and going further from here. Having the linter is good for future changes.

What was remaining is the expected output from get_uri, but as mentioned in #109 this can be done after this PR.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

Okay, that sounds good to me too. I just added a fix to look at the entrypoint and cmd for a recipe, and also there was a bug that the --force arg wasn't being passed from the client.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

I'm going to do some quick testing of the get_uri functions, because I can see some issues quickly. For execute, for example, if there is no loaded image it accepts "None" and this will trigger an error. Back in a bit!

@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

okay here is a quick explanation of the (not great) logic of get_uri.

The idea of get_uri is that if the user has loaded an image into the client, done with Client.load():

Client.load('docker://busybox')
docker://busybox

This loads the Image class to client.simage, where simage refers to "singularity image."

type(Client.simage)
spython.image.Image

The function "get_uri" that belongs to the base class determines if it's an instance or a container image based on this class.

def get_uri(self):
    ''' check if the loaded image object (self.simage) has an associated uri
        return if yes, None if not.
    '''
    if hasattr(self, 'simage'):
        if self.simage is not None:
            if self.simage.image not in ['', None]:
                # Concatenates the <uri>://<image>
                return str(self.simage)

But it's a poor design, because the Instance class has its own get_uri function:

instance.get_uri()
'instance://expressive_underoos_2092'

And the two classes are really doing almost the same thing, but one uses name, and the other image. I can probably fix that now to at least use a common variable (name and protocol).

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

Okay, I just pushed a fix so that (for run and exec) if an image isn't defined either from the user or client, we exit early. The design is a bit messy and we can work on further - when these tests are good I'm going to merge so the PR doesn't get too big.

@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

Let's merge this beast! Excellent work @Flamefire , and I'll get started on the changes to the recipe parser later this week. Tomorrow is probably a wash, but I will definitely have time Thursday Friday. Looking forward!

@vsoch vsoch merged commit 1ea23c5 into master May 28, 2019
@vsoch vsoch deleted the fix/misc branch May 28, 2019 18:10
@vsoch
Copy link
Member Author

vsoch commented May 28, 2019

Release on pypi https://pypi.org/project/spython/0.0.62/

Sign up for free to 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.

2 participants