Skip to content

Implementation of Job class #54

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

Merged
merged 4 commits into from
Jan 21, 2020
Merged

Implementation of Job class #54

merged 4 commits into from
Jan 21, 2020

Conversation

JohannesGaessler
Copy link
Contributor

@JohannesGaessler JohannesGaessler commented Jan 13, 2020

This PR adds an implementation for the Job class and its sub-classes.
I also laid some groundwork to be used for serialization in general.

  • Implement Job.
  • Implement JobSchedulingConstraints.
  • Implement DockerContext
  • Implement DockerConstraints
  • Add unit tests.

@JohannesGaessler JohannesGaessler marked this pull request as ready for review January 15, 2020 23:35
@JohannesGaessler JohannesGaessler requested review from ammen99, jooe1, FellowPlanter, nikolatzotchev and a team and removed request for a team January 15, 2020 23:36
docker_constraints: DockerConstraints,
label: str = None):
pass
self._uid: str = None
Copy link
Member

Choose a reason for hiding this comment

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

How will we generate the uid, i fount this online, but maybe we can just start with 1 and increment for each new job.

Copy link
Member

Choose a reason for hiding this comment

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

"If the job has not been queued yet, it has no UID." aa I see.

@nikolatzotchev
Copy link
Member

@JohannesGaessler I wanted to ask, what IDE are you using?

@JohannesGaessler
Copy link
Contributor Author

I am using PyCharm Community Edition.

@nikolatzotchev
Copy link
Member

Do you know how to run every test in a dir, I tried also in the terminal using

python -m unittest discover <test_directory>
but it runs 0 test, the same in pycharm.

Copy link
Member

@nikolatzotchev nikolatzotchev left a comment

Choose a reason for hiding this comment

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

To me everything looks good, all test are ok. I don't have a lot of experience with python so it will be good if other people check it as well.

@JohannesGaessler
Copy link
Contributor Author

Do you know how to run every test in a dir, I tried also in the terminal using

I usually just do it via setup.py
I pushed a fixup where I quickly threw something together.
To run all tests, run python setup.py test

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

Neat. It took me awhile to figure out why __eq__ is needed, but it definitely makes sense for unit tests.

@ammen99 ammen99 mentioned this pull request Jan 16, 2020
Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

Ready for merge, just squash the fixup commits.

@nikolatzotchev
Copy link
Member

Can we merge this pls.

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

Good that this wasn't merged, I completely forgot to check.

The unit tests do not pass MyPy checking, and I think our tests should be strongly typed, just as our code is. I'll also add the corresponding checks to my CI pull request.

@JohannesGaessler
Copy link
Contributor Author

Do we really need type safety for our unit tests?
The whole point of type safety is to prevent unforeseen runtime errors.
This makes no difference for unit tests since runtime errors would cause your unit tests to fail so you would notice immediately.
However, if I wanted to make the unittests type safe I would no longer be able to use the mixin approach that I am using now.
The problem is that when you import a subclass of unittest.TestCase the unit tests of that class will be executed in your test suite as well.
As a consequence inheritance like JobTest -> SerializableTest -> TestCase would be a giant pain to implement unless you resort to copy-pasting code.

@ammen99
Copy link
Member

ammen99 commented Jan 19, 2020

Do we really need type safety for our unit tests?
The whole point of type safety is to prevent unforeseen runtime errors.

I am using an IDE (rather vim + plugins) where type safety helps me use autocomplete and also catch errors while I am typing. Hence I'd like to use type safe code everywhere.

Seeing that the type is wrong can help you realize your mistake faster if the test fails.

The problem is that when you import a subclass of unittest.TestCase the unit tests of that class will be executed in your test suite as well.

Right, we can make an exception for the mixin class. Just add # type: ignore at the top of the python module for the mixin class. (found it here python/mypy#545 and it seems to work on my computer)

seems like it doesn't work for when you try to import the class in other modules, I'll try to see how that can be fixed.

Final test: you can add @no_type_check to the methods of the mixin class so that they are not type-checked. This does not seem to affect the rest of the code.

@ammen99 ammen99 merged commit b4b0d10 into master Jan 21, 2020
@JohannesGaessler JohannesGaessler deleted the implementation-job branch January 21, 2020 17:21
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.

4 participants