You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/01/16 17:00:21 UTC
[GitHub] [airflow] vsoch opened a new pull request #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
vsoch opened a new pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191
This is a second attempt to add Singularity Container support to Apache Airflow by way of Singularity Python. I am using the previously created JIRA ticket 4030 (created in March 2019) as it is still relevant. I am a new contributor and largely not familiar with the community here (and yes I've read the guidelines) so I would appreciate support and kindness from the individuals that act as maintainers here, and any additional support from other folks that are also interested in this integration. Thank you!
Signed-off-by: Vanessa Sochat <vs...@stanford.edu>
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
<sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk merged pull request #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590026699
It could be a transient error or some temporary problem with master (despite of the protections we have in place it breaks occassionally). We do not see it now in the latest master, so I suggest to rebase to the latest master (it's green: https://travis-ci.org/apache/airflow/builds/653784320?utm_medium=notification&utm_source=github_status) and see if the problem is still there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586638274
I can try to address the conflicts, but I can't rebase it seems:
```
Auto-merging setup.py
CONFLICT (content): Merge conflict in setup.py
Auto-merging scripts/ci/in_container/entrypoint_ci.sh
Auto-merging docs/operators-and-hooks-ref.rst
Auto-merging Dockerfile
error: could not apply bacc3b54c... adding singularity operator and tests
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply bacc3b54c... adding singularity operator and tests
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r380838653
##########
File path: airflow/providers/singularity/operators/__init__.py
##########
@@ -0,0 +1,172 @@
+#
Review comment:
Sure, happy to do this - it looks like it's the same case for the testing files, so I'll follow suit there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368706304
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
My suspicion was confirmed (no newline was actually wanted) and I was able to fix all the static issues (static check step passed!)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367826765
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
+ shutil.rmtree(self.image)
+
+ # If the container failed, raise the exception
+ if result['return_code'] != 0:
+ message = result['message']
+ raise AirflowException('Singularity failed: %s' % message)
Review comment:
```suggestion
raise AirflowException(f'Singularity failed: {message}')
```
We can now take advantage of f-strings ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579359117
I think the third approach is best:
> If you need something pre-backed in the Docker image itself then I think we need a separate Dockerfile with FROM: singularity and some steps to build the dockerfile before docker-compose is run (and refer to the newly built image in Docker compose. My best bet if that is the case, that would be that you build and test such images locally for local testing and once it works I can help to integrate it in the CI scripts properly (You can try yourself - this will likely be just appropriate 'docker build' command executed only when --integration singularity is used)
because Singularity requires a fairly specific set of custom dependencies, and then takes a while to compile. I can generate this file locally and use the already existing bases on quay.io. Could you give me some pointers about where to write the Dockerfile, what example from the current codebase (link) is what I should try to copy the logic for?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368761263
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
> This is not logical to me - both of these files have a newline at the end, yet they are reported fixed.
It's rather logical IMHO - I think you are likely thinking about empty LINE at the end, but what the end-of-file rightfully complained about was extra EOL character - empty line and EOL character are two different things.
The end-of-file fixer will fail if it has either any number of empty lines at the end or lack of EOL character at the end of last line. Both cases are undesirable for various reasons (you cannot easily append to a file which has no EOL and extra empty line is superfluous and might hide/miss the true end of file in editor.
That's why it failed likely because you had an empty line at the end (technically empty line at the end is exactly TWO EOL characters). It expects exactly one EOL at the end of file - no more, no less.
> The command you provided didn't work
First of all - you did not run it with `--all-files` . That's why it did not fix your files - they were already committed, not staged.
As explained in our "Use pre-commits" documentation, `pre commit run` only works on staged files. In your case you tried to run in on already committed files. For that `pre-commit run --all-files` is appropriate because it runs it on all files, not only on staged files. That should have fixed your problem with automated fixing of the files. I even mentioned both of the versions in my previous comment.
For the docker build failure - you were extremely unlucky in this case. There was a release of an extra dependency that broke the master build (not our fault) - that's why you had the sql.h error. It's been fixed in one of the latest commit but you are currently behind by 26 commits. You should rebase your change to the latest master to get it working.
You should do that anyway before merge (the best idea is to rebase often). You can see an exact way how to rebase in our documentation here: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#how-to-rebase-pr
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368318024
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Do we need this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590050857
Thanks everyone :) It's just a little before 3:00am here but I still think I'm going to have a tiny dance party before I slip back to sleep. :tada:
Really looking forward to someone using this in the wild!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588437467
okay, I'll try that! It does seem like I'm between a rock and a hard place, glad that you are okay with disabling pylint here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576467652
> Also, could you give feedback on:
>
> > Title Validator — Wrong commit title: adding singularity operator and tests
>
> What is wrong about it?
I think it's quite clearly described - it's about the "commit title". As mentioned in the PR description:
> Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
Commit message should start with [AIRFLOW-NNNN] similarly as PR description. You can run `git commit --amend` to change it and then `git push --force` to push it. It's also quite clearly described in the "Pull Request guidelines": https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines with example:
> Preface your commit's subject & PR title with [AIRFLOW-XXX] COMMIT_MSG where XXX is the JIRA number. For example: [AIRFLOW-5574] Fix Google Analytics script loading.
I have a question @vsoch - it seems that for some reason it is quite difficult to follow (for you at least). We have not only good and comprehensive documentation but also we have spend quite a lot of time to have messages printed either by the pre-commit system, or docs and require additional explanation (mostly repeating what is in the documentation already) - and pretty much careful reading the error messages could (I think) prevent asking extra questions - that was at least the goal. The messages are designed to guide you and In my answers I mostly repeat the messages already written there when you ask the question and link to already existing documentation.
I am really curious why it happens. Maybe we can improve it somehow - is the documentation not written clearly enough? Is there any problem with surfacing the error messages ? Or maybe there is a problem with finding relevant information? Or maybe you need more time to get familiar with the tools we are using ?
We tried (with professional technical writer) to place the documentation exactly where it is supposed to be, plus we push it exactly at the place where problems happen (for example in pre-commit checks) and try to make it as clear as possible - but apparently you still have problems with finding it. I am really curious - would it be possible for you to explain how are you approaching it, how are you reading (or maybe not?) the messages you get and how you are understanding (or not) what we try to communicate.
As a novice user you are the best "test" for it - maybe also you can have some ideas, proposals or maybe even a Pull Request that can improve it and make the documentation/messages better?
Looking forward to more insight.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367826410
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
Review comment:
```suggestion
if self.auto_remove and os.path.exists(self.image):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367827686
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
+ shutil.rmtree(self.image)
+
+ # If the container failed, raise the exception
+ if result['return_code'] != 0:
+ message = result['message']
+ raise AirflowException('Singularity failed: %s' % message)
+
+ self.log.info('Output from command %s', result['message'])
+
+ def get_command(self):
+ if self.command is not None and self.command.strip().find('[') == 0:
+ commands = ast.literal_eval(self.command)
+ else:
+ commands = self.command
+ return commands
+
+ def on_kill(self):
+ if self.instance is not None:
+ self.log.info('Stopping Singularity instance')
+ self.instance.stop()
+
+ # If an image exists, clean it up
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
Review comment:
```suggestion
if self.auto_remove and os.path.exists(self.image):
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that it should be a dictionary with all keys and values being strings. It's really easy to add and helps a lot (no need to search for info in docstring) and we can run static types checks that can discover some nasty edge cases. More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring, so it's just a matter of adding them in the constructor like `param: expected_type = default_value` :) Also, type annotations should be in line with `:type _:` in docstrings.
Some more interesting will be:
```
start_command: Optional[Union[str, List[str]]] = None
environment: Optional[Dict[str, Any]] = None
options: Optional[List[str]] = None
```
As you can see the good type of annotation should bring as much information as it can. At first, it can seem to be unnecessary work but in the end, it creates a big value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367824705
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Would you mind adding type annotations? It makes the code easier to understand and Airflow is using mypy for static analysis.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588463321
@potiuk I tried this huge build multiple times before, and they all resulted in errors. I'm glad to try it again, but I've had much better luck with reading the errors and addressing the errors verbatim.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590049375
holy crap it passed! Woot!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579877224
okay so I'm tracing the kubernetes (runtime) as an example, and I have a quick question. In scripts/ci/in_container/entrypoint_ci.sh I see that given the kubernetes runtime, you are setting some variable for a container name to be the airflow name with suffix "-kubernetes."
```bash
if [[ ${RUNTIME:=""} == "kubernetes" ]]; then
unset KRB5_CONFIG
unset KRB5_KTNAME
export AIRFLOW_KUBERNETES_IMAGE=${AIRFLOW_CI_IMAGE}-kubernetes
AIRFLOW_KUBERNETES_IMAGE_NAME=$(echo "${AIRFLOW_KUBERNETES_IMAGE}" | cut -f 1 -d ":")
export AIRFLOW_KUBERNETES_IMAGE_NAME
AIRFLOW_KUBERNETES_IMAGE_TAG=$(echo "${AIRFLOW_KUBERNETES_IMAGE}" | cut -f 2 -d ":")
export AIRFLOW_KUBERNETES_IMAGE_TAG
fi
```
What isn't clear is where this container is built, and then where it gets used again? At this point we are already sitting inside the airflow-testing container after a command like this:
```
else if [[ ${RUNTIME:=} == "singularity" ]]; then
set +u
# shellcheck disable=SC2016
docker-compose --log-level INFO \
-f "${MY_DIR}/docker-compose/base.yml" \
-f "${MY_DIR}/docker-compose/backend-${BACKEND}.yml" \
-f "${MY_DIR}/docker-compose/runtime-singularity.yml" \
"${INTEGRATIONS[@]}" \
"${DOCKER_COMPOSE_LOCAL[@]}" \
run airflow-testing \
'/opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"' \
/opt/airflow/scripts/ci/in_container/entrypoint_ci.sh "${@}"
# Note the command is there twice (!) because it is passed via bash -c
# and bash -c starts passing parameters from $0. TODO: fixme
set -u
```
and this would suggest the container is brought up inside of this container? Wouldn't it make more sense to have a docker-compose run that runs some equivalent of airflow-testing but with Singularity installed inside?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that all keys and values should be strings. It's really easy to add and helps a lot (no need to search for info in docstring). More information here: https://docs.python.org/3/library/typing.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368562951
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Hey @vsoch. This is not strictly required, it's just helpful for you to get changes verified without waiting for Travis build.
It's up to you if you want to run pre-commit only for commits or also for push. The latter has the benefit that it will run checks for all commits you are pushing together in case you have more than one commit.
Here you have more infor: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576523641
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=h1) Report
> Merging [#7191](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/44769dc172ff0438240dc64b5e5f6cb7c62f8af1?src=pr&el=desc) will **decrease** coverage by `52.68%`.
> The diff coverage is `17.97%`.
[](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7191 +/- ##
===========================================
- Coverage 85.37% 32.69% -52.69%
===========================================
Files 723 759 +36
Lines 39546 40008 +462
===========================================
- Hits 33764 13080 -20684
- Misses 5782 26928 +21146
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ntrib/example\_dags/example\_singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3Npbmd1bGFyaXR5X29wZXJhdG9yLnB5) | `0% <0%> (ø)` | |
| [airflow/contrib/operators/singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9zaW5ndWxhcml0eV9vcGVyYXRvci5weQ==) | `21.05% <21.05%> (ø)` | |
| [...low/contrib/operators/wasb\_delete\_blob\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy93YXNiX2RlbGV0ZV9ibG9iX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [...flow/contrib/example\_dags/example\_qubole\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3F1Ym9sZV9zZW5zb3IucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...ample\_dags/example\_emr\_job\_flow\_automatic\_steps.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX2Vtcl9qb2JfZmxvd19hdXRvbWF0aWNfc3RlcHMucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...contrib/example\_dags/example\_papermill\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3BhcGVybWlsbF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/hooks/webhdfs\_hook.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy93ZWJoZGZzX2hvb2sucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/sensors/emr\_base\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvZW1yX2Jhc2Vfc2Vuc29yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/operators/gcs\_list\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9nY3NfbGlzdF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/operators/pig\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcGlnX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [629 more](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=footer). Last update [44769dc...5ab4861](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576470784
> The linting appears to be all set, but since Singularity is installed, the tests are obviously going to fail. Let me know how you would like to proceed - it's a non trivial thing to get Singularity installed from within Travis.
I don't think we have enough singularity experience to add it. We approached in various ways for other integrations:
- some of the integrations (mongo etc.) are installed as docker-compose images (for example here: https://github.com/apache/airflow/blob/master/scripts/ci/docker-compose/integration-mongo.yml)
- some of them (Kubernetes) are installed via kind (Kubernetes-in-Docker) - here: https://github.com/apache/airflow/blob/master/scripts/ci/in_container/kubernetes/setup_kind_cluster.sh
- some of them we install inside the docker container via regular Dockerfile image build instructions (minicluster/hive/hadoop) - https://github.com/apache/airflow/blob/3730c24c41470cd331c5109539ee2fa0c9f4e74a/Dockerfile#L152 https://github.com/apache/airflow/blob/master/Dockerfile#L185
I do not know if/whether any of such options is possible for Singularity - please take a look and see if this is possible to use either of the options above?
If we find integration too difficult - In a number cases we rely on "unit tests" where we mock the external systems and mark the tests as skipped - we have a custom marker `pytest.mark.integration` that does it: https://github.com/apache/airflow/blob/master/TESTING.rst#running-integration-tests . If you have `pytest.mark.integration("singularity")` as decorator, the test will be skipped unless you have INTEGRATION_SINGULARITY env variable defined. You can leave such tests in the code and manually test it when you have singularity (but it's best if we can figure out a way to add it to the docker-compose we are already running).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579881380
okay just to clarify - you want a Singularity + Airflow container run via a similar kind cluster? You said something about adding specific commands `--start-singularity` and `--stop-singularity` and I'm not sure what / where that is referring to, and how / why we would want to start or stop singularity (it's not a service, it's just a binary installed).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that it should be a dictionary with all keys and values being strings. It's really easy to add and helps a lot (no need to search for info in docstring) and we can run static types checks that can discover some nasty edge cases. More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368323150
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
okay I let that run and it failed, so I removed the hooks. If there needs to be some formatting let's talk about commands to run that (outside of the much more complex logic added by the pre-commit command).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368563479
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Also regarding the pre-commit hook - answer "yes" with the first question and wait a bit to get it build (in the version that is coming soon I hope it will have more meaningful progress indicator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that all keys and values should be strings. It's really easy to add and helps a lot (no need to search for info in docstring) and we can run static types checks that can discover some nasty edge cases. More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-593282253
Hello @vsoch - I spoke to co-organizer of Airflow Summit - @leahecole - and I think we can figure out something with regards you being speaker at the Summit. @leahecole -> can you please reach out to @vsoch and take it from here. I would love to make this happen!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590051343
BTW. @vsoch I am going to give a talk together with @aijamalnk on Diversity and building even more welcoming community at the Airflow Summit (http://airflowsummit.org). And I will certainly use some of the things we've learned during this conversation. But I have just a thought - since you are nearby (I guess :) ) maybe you would like to give some talk about your side of the experience (and a bit about Singularity). I think that might be cool.
We have CFP open : https://pretalx.com/apache-airflow-summit-bay-area-2020/cfp :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] leahecole commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
leahecole commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-593639001
Hi @vsoch! Jarek spoke super highly about this contribution and I'd love to talk about how we can get you to the summit - I just followed you on twitter (@leahecole there too) - I think one good way would be if you're willing to give a talk about it - I'd be happy to help you through submitting the talk proposal. Feel free to DM me there to talk more details and thanks for your Airflow contribution! :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586036255
What are these new errors about encoding / pragma? I literally didn't change any of these files....
```
pragma.................................................................................Failed
- hook id: fix-encoding-pragma
- duration: 0.14s
- exit code: 1
- files were modified by this hook
Removed encoding pragma from airflow/contrib/operators/singularity_operator.py
Removed encoding pragma from airflow/contrib/example_dags/example_singularity_operator.py
Removed encoding pragma from tests/contrib/operators/test_singularity_operator.py
```
The Dockerlint errors are a bit much, but I'll jump through the hoops :/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588461323
> One quick note about the docs for pylint - the files are now changed to have pylint_main.sh and pylint_tests.sh. Also, this unexpectedly started that huge build process that failed before. Given a .pylintrc file in some root and running pylint, it would be great to see a much simpler (not wrapped / abstracted) run of pylint on a single file.
I'd really recommend (again) pre-commits. They will run pylint on all modified files only. This is far better than having to run it manually - the will simply run pylint (or whatever of the other tests) automatically on all the files you modified when committing it. You don't even have to remember about it. And the reason for the build process is to make sure that you run it in the same environment as Travis CI and other people. The problems with tools like pylint is that the results depend often on installed libraries, pylint version etc. For example you need to have installed stubs for python libraries in order to do full static analysis. So there is huge variability depending on your local environment and you can simply get different results locally than on Travis. That's why we run pylint tests inside the docker container which mirrors this container that runs on Travis. This is the only way to make sure everyone runs in the same environment.
It happened many times that people reported "I see different errors than those on Travis" - and it turned out that they have different pylint or python version. This is the real benefit of the "huge build". you start to appreciate it when your team of contributors is totally distributed, maintain their own environment and consists of 600 individuals - each with their own setup.
Simply - it looks differently from the point of view of single person and differently from the point of view of team of few hundred people. Sometimes efficiency of team require some sacrifice of efficiency of single individual.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579443300
> Could you give me some pointers about where to write the Dockerfile, what example from the current codebase (link) is what I should try to copy the logic for?
I think the closest one is Kubernetes and I am leaning towards creating another RUNTIME (singularity). There are few of scripts to update. But it's not that hard at the end.
I think a custom `--start-singularity` `--stop-singularity` switches should be added similar to `--start-kind-cluster` `--stop-kind-cluster`.
It's best if you look for RUNTIME capital wherever it's used but here is the guide
Breeze (for local development and testing your solution):
- https://github.com/apache/airflow/blob/master/breeze-complete#L40 -> options to add runtime are added here amongst others
- https://github.com/apache/airflow/blob/master/breeze#L203 -> Displaying chosen RUNTIME
- https://github.com/apache/airflow/blob/master/breeze#L595 -> Flags of Breeze
- https://github.com/apache/airflow/blob/master/breeze#L1007 -> that's where RUNTIME leads to adding extra docker compose
- https://github.com/apache/airflow/blob/master/breeze#L391 -> help describing the options.
Docker Compose:
- https://github.com/apache/airflow/blob/master/scripts/ci/docker-compose/runtime-kubernetes.yml - similar should be created for singularity and used in Breeze.
In container:
- https://github.com/apache/airflow/blob/master/scripts/ci/in_container/entrypoint_ci.sh#L164 - entrypoint for the CI where Kubernetes is started after choosing the RUNTIME="kubernetes" here is where the setup for singularity can be done. It runs https://github.com/apache/airflow/blob/master/scripts/ci/in_container/kubernetes/setup_kind_cluster.sh under the hood.
That's it for interactive "local" breeze session. T
Then if you run tests the script goes further. It builds airflow kubernetes image and deploys it to kubernetes. In interactive session, the deployment should be run manually as described in https://github.com/apache/airflow/blob/master/TESTING.rst#running-tests-with-kubernetes
but if you run tests in travis (RUN_TESTS="true") those steps are executed manually.
https://github.com/apache/airflow/blob/master/scripts/ci/in_container/entrypoint_ci.sh#L218 you have the deployment of image to kubernetes: https://github.com/apache/airflow/blob/master/scripts/ci/in_container/deploy_airflow_to_kubernetes.sh and one of the scripts it runs https://github.com/apache/airflow/blob/master/scripts/ci/in_container/kubernetes/docker/rebuild_airflow_image.sh - that's where the Kubernetes image is built.
I hope that's enough leads.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-589285097
Quick question - I don't see any reference to the singularity operator [here](https://travis-ci.org/apache/airflow/jobs/653112190) but the tests failed - do you know what's up? I'll focus on just the vanilla postgres for now, that seems to have errors for the operator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r380544005
##########
File path: airflow/providers/singularity/operators/__init__.py
##########
@@ -0,0 +1,172 @@
+#
Review comment:
We should avoid adding code in `__init__.py` . Each operator should have their own file. Adding code to `__init__.py` causes the side effect that the code is imported and the imports are further processed when the package is imported. This is very bad as it is implicit behavior not expected by the one who imports it and it causes all the various problems with chaining loading of unnecessary imports which slows down application startup. We are just now voting in the devlist on removal of `airflow.__init__.py` as this is very bad example on how things should not be done.
Please move it to separate class - this should also solve the problem with documentation building. One of the side effects of adding imports to __init__.py is tha the imported AirflowException class is now defined in two places.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579878775
Oh wait I see, the "deploy_airflow_to_kubernetes.sh" detects the runtime and triggers the build to happen via: https://github.com/apache/airflow/blob/master/scripts/ci/in_container/kubernetes/docker/rebuild_airflow_image.sh. Let me see if I can mirror that with Singularity.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586021498
okay to so to update everyone following (and future me reading back to remember this) we are first trying a simple approach to install Singularity in the base container, and then the tests will run for all the various environments without a special runtime. The reason is because Singularity is not a service (like Docker has a daemon) but rather just a binary that can create instances / containers / otherwise. This first test will just be to see if singularity is detected in the host container when the executors run, granted that the resolved merge for setup.py didn't result in linting errors or what not.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368325107
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
okay I let that run and it failed, so I removed the hooks. If there needs to be some formatting let's talk about commands to run that (outside of the much more complex logic added by the pre-commit command).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588482545
Hmm, so I definitely added the disable line, but it doesn't seem to be taking:
https://github.com/apache/airflow/pull/7191/commits/cef6c077927e996f5c910c872a775850a25596f0
It's triggering for line 65, so I suppose I can try adding it there...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368762478
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Ah yes, I see the difference between newline and end of line, and also the reasons for the pre-commit run not detecting files, thanks for the clarification! And for the container build, haha. I should expect this sort of thing. Thankfully I could just use the CI output to debug and fix the issues, so the static checks and docs are all good now.
The issue remaining is my last post - the fact that we cannot test the singularity operator without installing Singularity, which is a non trivial thing to do on travis (you usually need a machine base with a lot of custom libraries and GoLang). Please let me know how to proceed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368563479
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Also regarding the pre-commit hook - answer "yes" with the first question and wait a bit to get it build (in the version that is coming soon I hope it will have more meaningful progress bar.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586638581
Note that there were conflicts with setup.py before my set of last commits, and I resolved them here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that it should be a dictionary with all keys and values should be strings. It's really easy to add and helps a lot (no need to search for info in docstring) and we can run static types checks that can discover some nasty edge cases. More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590053213
I see, pity, let's see. It's the first-time event so we do not know what to expect and for now do not foresee reimbursements, but if we are successful with finding sponsors, who knows, maybe we will introduce a diversity fund. I will keep you updated :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587500292
Yesterday we also added "example_dags/__init__.py" to list of known issues related to documentation:
https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588442023
One quick note about the docs for pylint - the files are now changed to have pylint_main.sh and pylint_tests.sh. Also, this unexpectedly started that huge build process that failed before. Given a .pylintrc file in some root and running pylint, it would be great to see a much simpler (not wrapped / abstracted) run of pylint on a single file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590033385
one transient error - but I restarted it. Looks good !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588448150
okay, I found plenty of examples and copied those, so I didn't need to run it manually. Let's hope that works!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576523641
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=h1) Report
> Merging [#7191](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/40fbadc3f6b737ae0e387302f7ffbd63ca3c943e?src=pr&el=desc) will **increase** coverage by `54%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7191 +/- ##
==========================================
+ Coverage 32.8% 86.81% +54%
==========================================
Files 892 893 +1
Lines 42173 42186 +13
==========================================
+ Hits 13836 36622 +22786
+ Misses 28337 5564 -22773
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/bin/airflow](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9iaW4vYWlyZmxvdw==) | `84.61% <0%> (ø)` | |
| [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `86.95% <0%> (+0.72%)` | :arrow_up: |
| [airflow/utils/db.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9kYi5weQ==) | `98.31% <0%> (+0.84%)` | :arrow_up: |
| [airflow/bin/cli.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9iaW4vY2xpLnB5) | `94.73% <0%> (+2.1%)` | :arrow_up: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `88.51% <0%> (+3.37%)` | :arrow_up: |
| [airflow/exceptions.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGNlcHRpb25zLnB5) | `100% <0%> (+3.57%)` | :arrow_up: |
| [airflow/utils/sqlalchemy.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9zcWxhbGNoZW15LnB5) | `84.93% <0%> (+4.1%)` | :arrow_up: |
| [airflow/utils/decorators.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9kZWNvcmF0b3JzLnB5) | `90.47% <0%> (+4.76%)` | :arrow_up: |
| [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `92.25% <0%> (+5.63%)` | :arrow_up: |
| [airflow/www/forms.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy93d3cvZm9ybXMucHk=) | `100% <0%> (+7.27%)` | :arrow_up: |
| ... and [760 more](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=footer). Last update [40fbadc...c089430](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576470774
Ah understood, I thought it was for the PR title, I’ll update the message for the next fix I do.
I think the documentation is good, there are just too many moving parts for a new contributor. I have limited bandwidth to read into the minute details so I tend to be responsive over proactive in terms of reading everything first, and I would say this tends to be the case for a large subset of contributors - we understand the basics and rely on the CI to tell us what to fix, and don’t expect all the other bells and whistles with new tools and steps we have to learn for just this PR. For this particular PR I found when I tried to follow the docs it was more trouble than just looking at the errors in the CI and fixing them one by one. When I largely ignored the docs and just looked at the CI and tried to fix things on my own based on that I was finally able to make progress. Your links and verbose re-explanation of current docs I know are done with good intention, but a large chunk sort of just confused me more.
If it makes you feel better, just chock it up to me being an idiot and I promise I’ll never bother this community again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590029643
Alright, rebase is just done! I think it's likely something transient, because the rebase just had changes to UPDATING.md (note that right before the set of last pushes I rebased when there was a conflict with setup.py). So let's hope that the transient error doesn't show up again!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368536213
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Also, do I want to install just the pre-commit or also the pre-push hooks?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585964299
> 1. installing Singularity as a binary inside a container (these bases already exist)
> 2. running the container via docker-compose
> 3. But then running tests inside of that container
>
> Is this possible?
I see now !. In this case I think the easiest is to add the binary to Docker image and running it from there. Then there is no need to run it as separate image/runtime. That will be way simpler. We already do that for the minicluster - for hadoop tests:
Docker installation here: https://github.com/apache/airflow/blob/master/Dockerfile#L184
And starting the cluster here: https://github.com/apache/airflow/blob/master/scripts/ci/in_container/entrypoint_ci.sh#L140
J.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586067049
High level feedback since this was a topic earlier:
- the extra linting for the Dockerfile, which comes down to fairly trivial things like using cd in a command for one layer and using curl instead of wget, has led to several additional errors that are frustrating to deal with. If I knew the project perfectly sure I might have known these in advance, but for a new contributor that gets these bugs (which are somewhat subjective) that then has to figure them out and fix, it's an extra pain that simply doesn't need to exist.
- given the long duration of a PR, the addition of new linters / tests makes it even more confusing.
I'm happy to keep providing this feedback, and if you don't want it let me know!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368684397
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
I'm going to manually go through the messages on Travis, and try to manually fix things. I see no other way to do this, this automated approach does not work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590033396
:crossed_fingers:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368024809
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
Review comment:
Accepted (and edited locally!) I am going to do a rebase as I'm supposed to so I'm being very careful about random commits.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576472830
But one quick note - regardless of the testing troubles, the support on this PR has been absolutely excellent, night and day when compared to my previous experience. I'm really appreciative of that :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368029609
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
I don't see examples for other contrib operators (just looked at Docker) and I don't have experience using them, so I need to ask for help here. I can see that the imports are generally `from typing import ...` but I'm not sure how to use them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368684760
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
This is not logical to me - both of these files have a newline at the end, yet they are reported fixed.
Since the tests file wasn't changed, this suggests that it _doesn't_ want the newline...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587823490
docs build passed! Tiny progress :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590052010
Ah, I love the computer history museum! I’m actually a remote worker to Stanford based out of Colorado, so there is still an hour or so drive, few hour flight, and expensive Bay Area hotels in the way of me participating. Maybe in the future if there is an option to join remotely or get reimbursed, I’d be able to come. And in the meantime I can definitely look over content and give feedback, if that might help.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585939693
Somehow I missed it completely. Sorry. Responding now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586998761
Here is guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#documentation Please look at the "Known issues" section.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367826023
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
Review comment:
If a user has to provide a `command` why set `command=None` in the constructor?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368024283
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
Review comment:
Singularity containers can usually serve multiple purposes, and especially for scientific use cases, they tend to be used with "exec" with custom entry points. So the thinking here was that the user should be able to instantiate the operator and then modify the command to be different things. This might be incorrect / not common usage of airflow (for example, if it would be expected to instantiate one container instance per command) in which case I agree, and we should remove the command=None and check for it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576467652
> Also, could you give feedback on:
>
> > Title Validator — Wrong commit title: adding singularity operator and tests
>
> What is wrong about it?
I think it's quite clearly described - it's about the "commit title". Commit title is the first line in commit message. As mentioned in the PR description:
> Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
Commit message should start with [AIRFLOW-NNNN] similarly as PR description. You can run `git commit --amend` to change it and then `git push --force` to push it. It's also quite clearly described in the "Pull Request guidelines": https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines with example:
> Preface your commit's subject & PR title with [AIRFLOW-XXX] COMMIT_MSG where XXX is the JIRA number. For example: [AIRFLOW-5574] Fix Google Analytics script loading.
I have a question @vsoch - it seems that for some reason it is quite difficult to follow (for you at least). We have not only good and comprehensive documentation but also we have spend quite a lot of time to have messages printed either by the pre-commit system, or docs and require additional explanation (mostly repeating what is in the documentation already) - and pretty much careful reading the error messages could (I think) prevent asking extra questions - that was at least the goal. The messages are designed to guide you and In my answers I mostly repeat the messages already written there when you ask the question and link to already existing documentation.
I am really curious why it happens. Maybe we can improve it somehow - is the documentation not written clearly enough? Is there any problem with surfacing the error messages ? Or maybe there is a problem with finding relevant information? Or maybe you need more time to get familiar with the tools we are using ?
We tried (with professional technical writer) to place the documentation exactly where it is supposed to be, plus we push it exactly at the place where problems happen (for example in pre-commit checks) and try to make it as clear as possible - but apparently you still have problems with finding it. I am really curious - would it be possible for you to explain how are you approaching it, how are you reading (or maybe not?) the messages you get and how you are understanding (or not) what we try to communicate.
As a novice user you are the best "test" for it - maybe also you can have some ideas, proposals or maybe even a Pull Request that can improve it and make the documentation/messages better?
Looking forward to more insight.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368025594
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
+ shutil.rmtree(self.image)
+
+ # If the container failed, raise the exception
+ if result['return_code'] != 0:
+ message = result['message']
+ raise AirflowException('Singularity failed: %s' % message)
+
+ self.log.info('Output from command %s', result['message'])
+
+ def get_command(self):
+ if self.command is not None and self.command.strip().find('[') == 0:
+ commands = ast.literal_eval(self.command)
+ else:
+ commands = self.command
+ return commands
+
+ def on_kill(self):
+ if self.instance is not None:
+ self.log.info('Stopping Singularity instance')
+ self.instance.stop()
+
+ # If an image exists, clean it up
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
Review comment:
Fixed this as well, thank you for the suggestion!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585967157
Perfecto! I'll give this a shot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368311967
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
Review comment:
okay, that works for me!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585946960
> I assume we can start singularity from the host and be able to forward this connection to inside the airlfow-testing container so that we can connect to it. With Kind - we are starting it from within the container (by forwarded docker socket) , but it could be started from the host as well (mongo, kerberos and others are started from the host using docker-compose configuration and then we can connect to them from the "airflow-testing" by specifying their service names (mongo/kerberos etc).
Ah yes this is what I wanted to ask about, specifically:
> I assume we can start singularity from the host and be able to forward this connection to inside the airlfow-testing container so that we can connect to it.
Singularity is different from docker - it doesn't have a service or daemon, it's akin to an executable. It would be installed inside a container, and it wouldn't work to "start on the host and forward."
> started from the host using docker-compose configuration and then we can connect to them from the "airflow-testing" by specifying their service names (mongo/kerberos etc).
You mean to say that we start a container with Singularity installed via docker-compose, and then run as a service? I think it would work to run things internally, I'm not sure the extent to which you could issue interactions from the outside. This I think is something that we could do:
1. installing Singularity as a binary inside a container (these bases already exist)
2. running the container via docker-compose
3. But then running tests inside of that container
Is this possible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368585955
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Also in case of your problems in the latest build - just read the output messages in the failed "static checks". I think the messages there are pretty clear, but maybe you need some more guidance here.
You can read more of the pre-commit checks and how we are using them in https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks - there is a list of pre-commit checks there and explanation what is the expectation to .
But to additionally describe it here and give you additional guidance - my observations for the current errors you have:
- The end-of-file/isort you can have pre-commit fixing it for you locally (and you will just have to commit it (end-of-file and isort for example). We have the requirements for isort-order of imports (isort is the tool that sorts them automatically)
- In other cases you need to fix it because there is - sometimes potential - problem in the code (for example in pylint, or the checks for proper sorting of dependencies in setup.py.
- In case of the sorting error - the dependencies in setup.py are alphabetically sorted so you should add your deps in sorted order.
- In case of pylint errors - you seem to have too many arguments in one of the operators - you can solve it either by decreasing the number of operators or by adding appropriate pylint disable. You can see how to do it in our docs - there is explanation when and how you should manually disable the checks and explanation how to do it: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pylint-static-code-checks
- The other "test related" pylint errors - that's a clear programming error that pylint detected - there is no "compat" in module "tests" in your import (we dropped compat in master as we dropped python 2 support) and SingularityOperator is not imported it seems.
> let's talk about commands to run that (outside of the much more complex logic added by the pre-commit command).
It is super easy: You can run those pre-commits manually (that's the power of pre-commit framework.
for example you can run:
`pre-commit run end-of-file-fixer`
for the changes you have locally added, or even
`pre-commit run end-of-file-fixer --all-files'
And it will run the very same pre-commit checks as in Travis for all files. So you have easy way to reproduce it locally.
This is explained in detail in "Using pre-commit hooks" - including examples: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#using-pre-commit-hooks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588435931
> How would you like me to address this?
>
> ```
> PATH=/root:/usr/local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/hive/bin:/usr/local/go/bin:/opt/gcloud/bin
>
> ************* Module airflow.providers.singularity.operators.singularity
>
> airflow/providers/singularity/operators/singularity.py:65:4: R0913: Too many arguments (12/10) (too-many-arguments)
> ```
>
> I had added working_dir to kwargs, but then during the test there was an error that the argument wasn't found. So I added it, but now I'm over.
The pylint defats we have are 'reasonable' i.e work in most cases but if you have a justified case and do not think refactoring / using a structure instead of n-parameters you could disable pylint check - this specific check for this statement only by adding appropriate # pylint: disable comment. This is a sign from the programmer that this is a deliberate decision and theat it is conscious choice to disable tis rule. You can find plenty of examples in the code and more detailed explanation including some guidance on the decision making in the https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst in pylint section
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576523641
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=h1) Report
> Merging [#7191](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/44769dc172ff0438240dc64b5e5f6cb7c62f8af1?src=pr&el=desc) will **decrease** coverage by `52.68%`.
> The diff coverage is `17.97%`.
[](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7191 +/- ##
===========================================
- Coverage 85.37% 32.69% -52.69%
===========================================
Files 723 759 +36
Lines 39546 40008 +462
===========================================
- Hits 33764 13080 -20684
- Misses 5782 26928 +21146
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ntrib/example\_dags/example\_singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3Npbmd1bGFyaXR5X29wZXJhdG9yLnB5) | `0% <0%> (ø)` | |
| [airflow/contrib/operators/singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9zaW5ndWxhcml0eV9vcGVyYXRvci5weQ==) | `21.05% <21.05%> (ø)` | |
| [...low/contrib/operators/wasb\_delete\_blob\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy93YXNiX2RlbGV0ZV9ibG9iX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [...flow/contrib/example\_dags/example\_qubole\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3F1Ym9sZV9zZW5zb3IucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...ample\_dags/example\_emr\_job\_flow\_automatic\_steps.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX2Vtcl9qb2JfZmxvd19hdXRvbWF0aWNfc3RlcHMucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...contrib/example\_dags/example\_papermill\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3BhcGVybWlsbF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/hooks/webhdfs\_hook.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy93ZWJoZGZzX2hvb2sucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/sensors/emr\_base\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvZW1yX2Jhc2Vfc2Vuc29yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/operators/gcs\_list\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9nY3NfbGlzdF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/operators/pig\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcGlnX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [629 more](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=footer). Last update [44769dc...5ab4861](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586424350
Questions:
- where does example_singularity_operator.py go - it used to be under contrib/example_dags but now, that entire folder is empty?
- this error is triggering for the docs (is it related to the above)? I don't see any script_files with grep, nor do I see how super() relates to the example_singularity_operator.py file.
```
writing additional pages... search/usr/local/lib/python3.6/site-packages/sphinx_rtd_theme/search.html:20: RemovedInSphinx30Warning: To modify script_files in the theme is deprecated. Please insert a <script> tag directly in your theme instead.
{{ super() }}
done
```
These docs tests were passing before (but nothing has changed) so what has changed that I don't know about?
For the mock tests, the author that helped me put "awesome-image" and "awesome-command" in the mock tests - I'm not familiar enough with setting up mocks to understand how this mock is supposed to work (it's not supposed to use real singularity?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368682449
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
The command you provided didn't work, it said there weren't files. Here is the complete output of running this command again, I see the container(s) building, and then failure. Most of the checks that (I think) are supposed to give me feedback are skipped...
https://pastebin.com/h3v7mwFy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576475634
> If it makes you feel better, just chock it up to me being an idiot and I promise I’ll never bother this community again.
First of all it's not a matter of making me feel better at all. I certainly don't think of you as an idiot, and I would never, ever call anyone idiot, just because that person has no time, or bandwidth or capacity, to read and understand the TON of documentation we have. And if anything - that's a sign that maybe we should do better. You seem to be competent and you know how to program and fix things on your own and do your best to do it - I certainly cannot blame you that you do not know our complex setup.
And Airflow IS a complex project. It has a LOT of moving parts and my goal is to make it approachable to even one-time contributors and my ultimate goal is to make it as easy and painless as possible - while keeping the high bar on quality, test automation, check automation etc.
I am just curious what are the obstacles to that - and whether we could make it somewhat easier to someone like you who just comes to the project from outside for a short time. That's why I am looking for some suggestions/areas of improvements. I know the project too well myself to see the obstacles - there is even a term for that "Expert Blindness" - http://mnav.com/expert-blindness/ that's why I am looking for help and suggestions of people like you.
To have the project much easier approachable is my goal from the very beginning of the project and - believe me - a year ago it took literally days to set it all up so that you could run tests. Now (if we do not have sudden breakage of master by 3rd party dependencies) you can have it up and running in 10 minutes (with fast network).
I think so far - by looking of what you've done, I will make a few improvements:
- I am going to make "Breeze" THE default dev environment (we have it much lighter as of a few weeks) and if you had it installed (by simply running ./breeze command), then pre-commit would just work out-of-the box
or (and?)
- we can direct new contributors to simply follow the CI messages - they are descriptive enough and should be good to start
I already added a few improvements to Contributing and we even have this PR in progress to explain how communication is expected to look like : https://github.com/apache/airflow/pull/7204 - I will be happy if you add your comments there.
WDYT? Anything else you think we can improve to make it more approachable?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576477396
> But one quick note - regardless of the testing troubles, the support on this PR has been absolutely excellent, night and day when compared to my previous experience. I'm really appreciative of that :)
Thanks for the kind words :). The trouble with that it that we can't afford to spend that much time for every single new person coming :(. The project needs to be scalable. That's why my goal is to have it all as smooth as possible and as easy to self-study and self-discover as possible - only reaching out to mentors/committers/other experienced contributors when really needed.
Of course you can't do it from day 1 or even day 5, but my hope is that if we relentlessly work on it and improve it and also get a lot of feedback from people like you - we can approach this "ideal" where everything is self-explanatory and easy to follow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-589639710
> looks like the error linked above was one off - a later build didn't trigger it.
Yeah. We still have some flaky tests - much less than before but they happen sometimes due to resource problems we have with Travis :(. We are close to migrate to Github Actions but it's one of the things that gets delayed as we are busy with other higher-priority stuff. It's annoying but it's not a blocker
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586039637
okay I'm going to assume that this "no pragma" is new and just remove the lines from the top of those files. I also used the docker linting container locally with the linting configuration and container, and it seems to produce no output after some fixes so I'm hoping this works. I don't necessarily think the changes are better - it didn't let me use cd in a multi line statement, and instead I had to change to artificially add a WORKDIR between, so now there are two layers instead of just one. But I guess the linting passes, lol!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586639114
Conflict resolved - it was the same line as before, another package was added! I added a new line (it was getting too long) so hopefully that won't make the linter unhappy...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586640329
> Conflict resolved - it was the same line as before, another package was added! I added a new line (it was getting too long) so hopefully that won't make the linter unhappy...
yep. it is a very active project. so conflicts are expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586651037
Can you show me where to add in the toc tree? I thought I grepped for similar providers and added it, but it's not happy:
> opt/airflow/docs/_api/example_singularity_operator/index.rst: WARNING: document isn't included in any toctree
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r380544005
##########
File path: airflow/providers/singularity/operators/__init__.py
##########
@@ -0,0 +1,172 @@
+#
Review comment:
We should avoid adding code in `__init__.py` . Each operator should have their own file. Adding code to `__init__.py` causes the side effect that the code is imported and the imports are further processed when the package is imported. This is very bad as it is implicit behavior not expected by the one who imports it and it causes all the various problems with chaining loading of unnecessary imports which slows down application startup. We are just now voting in the devlist on removal of `airflow.__init__.py` as this is very bad example on how things should not be done.
Please move it to separate class - this should also solve the problem with documentation building. One of the side effects of adding imports to __init__.py is tha the imported AirflowException class is now imported in two places.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586178588
I totally agree on all points! I can’t imagine what managing a PR board like this must be like.
I’m mostly happy that the pre-checks passed and we are inching forward! I’ll be able to look at this again later today.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587082403
Ah thank you! Just added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587354436
Hey @vsoch a bit unrelated, but wanted you to know that we have just merged this update to CONTRIBUTING documentation to make ti easier for people like you to understand the dynamics of our project's communication: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#how-to-communicate
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587936273
Actually it looks like it wasn't happy with working_dir as a kwargs, I'll try adding it to the type definitions to see if that changes anything...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586633715
and you need t rebase as well ontop of latest master & you have conflict with setup.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289406
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
Review comment:
In case of Airflow the instantiated operator will be the same same thing that will be executed as a task. I think we should make `command` a required argument, this will help users not familiar with Singularity to understand what has to be provided without deep dive into code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588357686
How would you like me to address this?
```
PATH=/root:/usr/local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/hive/bin:/usr/local/go/bin:/opt/gcloud/bin
************* Module airflow.providers.singularity.operators.singularity
airflow/providers/singularity/operators/singularity.py:65:4: R0913: Too many arguments (12/10) (too-many-arguments)
```
I had added working_dir to kwargs, but then during the test there was an error that the argument wasn't found. So I added it, but now I'm over.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587023735
Oh. I looked deeper. You need to create `airflow/airflow/providers/singularity/example_dags/__init__.py` file Otherwise, Sphinx cannot create the package and therefore also exclude this file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590012811
@potiuk for the recent tests, I'm seeing a redirect (302) for an address that (used to be?) 200? I don't see any singularity operator fails to debug so I don't have any new commits to push to re-test. Is it possible for someone on your team to trigger a re-run, in the case of a one time spoof, or to take a look into the redirects? Here is the output that I'm referencing:
```
tests/www/test_views.py:98: in check_content_in_response
self.assertEqual(resp_code, resp.status_code)
E AssertionError: 200 != 302
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576470784
> The linting appears to be all set, but since Singularity is installed, the tests are obviously going to fail. Let me know how you would like to proceed - it's a non trivial thing to get Singularity installed from within Travis.
I don't think we have enough singularity experience to add it. We approached in various ways for other integrations:
- some of the integrations (mongo etc.) are installed as docker-compose images (for example here: https://github.com/apache/airflow/blob/master/scripts/ci/docker-compose/integration-mongo.yml)
- some of them (Kubernetes) are installed via kind (Kubernetes-in-Docker) - here: https://github.com/apache/airflow/blob/master/scripts/ci/in_container/kubernetes/setup_kind_cluster.sh
- some of them we install inside the docker container via regular Dockerfile image build instructions (minicluster/hive/hadoop) - https://github.com/apache/airflow/blob/3730c24c41470cd331c5109539ee2fa0c9f4e74a/Dockerfile#L152 https://github.com/apache/airflow/blob/master/Dockerfile#L185
I do not know if/whether any of such options is possible for Singularity - please take a look and see if this is possible to use either of the options above?
If we find integration too difficult - In a number cases we rely on "unit tests" where we mock the external systems and mark the integration tests as skipped - we have a custom marker `pytest.mark.integration` that does it: https://github.com/apache/airflow/blob/master/TESTING.rst#running-integration-tests . If you have `pytest.mark.integration("singularity")` as decorator, the test will be skipped unless you have INTEGRATION_SINGULARITY env variable defined. You can leave such tests in the code and manually test it when you have singularity (but it's best if we can figure out a way to add it to the docker-compose we are already running).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368585955
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Also in case of your problems in the latest build - just read the output messages in the failed "static checks". I think the messages there are pretty clear, but maybe you need some more guidance here.
You can read more of the pre-commit checks and how we are using them in https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks - there is a list of pre-commit checks there and explanation what is the expectation.
But to additionally describe it here and give you additional guidance - my observations for the current errors you have:
- The end-of-file/isort you can have pre-commit fixing it for you locally (and you will just have to commit it (end-of-file and isort for example). We have the requirements for isort-order of imports (isort is the tool that sorts them automatically)
- In other cases you need to fix it because there is - sometimes potential - problem in the code (for example in pylint, or the checks for proper sorting of dependencies in setup.py.
- In case of the sorting error - the dependencies in setup.py are alphabetically sorted so you should add your deps in sorted order.
- In case of pylint errors - you seem to have too many arguments in one of the operators - you can solve it either by decreasing the number of operators or by adding appropriate pylint disable. You can see how to do it in our docs - there is explanation when and how you should manually disable the checks and explanation how to do it: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pylint-static-code-checks
- The other "test related" pylint errors - that's a clear programming error that pylint detected - there is no "compat" in module "tests" in your import (we dropped compat in master as we dropped python 2 support) and SingularityOperator is not imported it seems.
> let's talk about commands to run that (outside of the much more complex logic added by the pre-commit command).
It is super easy: You can run those pre-commits manually (that's the power of pre-commit framework.
for example you can run:
`pre-commit run end-of-file-fixer`
for the changes you have locally added, or even
`pre-commit run end-of-file-fixer --all-files'
And it will run the very same pre-commit checks as in Travis for all files. So you have easy way to reproduce it locally.
This is explained in detail in "Using pre-commit hooks" - including examples: https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#using-pre-commit-hooks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368654012
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
okay I'll run it again, not sure it will help...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368525347
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
The hook is often expected to fail but it will apply required changes (sort of imports, no spaces at the end of line, etc) to your file. After that you should add changed files and try to commit one more time.
It can "hang" a little bit on forcing response but then it should go on. @potiuk
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586640931
Okay the changes I've just made:
- moved `airflow/contrib/operators/singularity_operator.py` to `airflow/providers/singularity/operators/__init__.py` and added `__init__.py` in folder above to make module.
- updated docs to point to - :mod:`airflow.providers.operators.singularity`
- added `airflow/providers/singularity/operators/index` to the autoapi_templates index file (I just figured this out with grep, I had already triggered the new testing with fixes to setup.py
- for the mock tests, I have no idea how to do this, but I changed the image / commands to actual commands / images that might be used. As for the rest, no clue.
I don't know what devliscie is, just fyi.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586644781
> Sorry - devlist@airflow.apache.org - see CONTRIB.rst. I was typing it on my tablet and autocomplete kicked in.
Haha, no worries :) I was thinking it was some kind of special club that I am far too old and uncool for. Which is okay too! :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586641854
Sorry - devlist@airflow.apache.org - see CONTRIB.rst. I was typing it on my tablet and autocomplete kicked in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585943248
> okay just to clarify - you want a Singularity + Airflow container run via a similar kind cluster? You said something about adding specific commands `--start-singularity` and `--stop-singularity` and I'm not sure what / where that is referring to, and how / why we would want to start or stop singularity (it's not a service, it's just a binary installed).
I assume we can start singularity from the host and be able to forward this connection to inside the airlfow-testing container so that we can connect to it. With Kind - we are starting it from within the container (by forwarded docker socket) , but it could be started from the host as well (mongo, kerberos and others are started from the host using `docker-compose` configuration and then we can connect to them from the "airflow-testing" by specifying their service names (mongo/kerberos etc).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-588461323
> One quick note about the docs for pylint - the files are now changed to have pylint_main.sh and pylint_tests.sh. Also, this unexpectedly started that huge build process that failed before. Given a .pylintrc file in some root and running pylint, it would be great to see a much simpler (not wrapped / abstracted) run of pylint on a single file.
I'd really recommend (again) pre-commits. They will run pylint on all modified files only. This is far better than having to run it manually - the will simply run pylint (or whatever of the other tests) automatically on all the files you modified when committing it. You don't even have to remember about it. And the reason for the build process is to make sure that you run it in the same environment as Travis CI and other people. The problems with tools like pylint is that the results depend often on installed libraries, pylint version etc. For example you need to have installed stubs for python libraries in order to do full static analysis. So there is huge variability depending on your local environment and you can simply get different results locally than on Travis. That's why we run pylint tests inside the docker container which mirrors this container that runs on Travis. This is the only way to make sure everyone runs in the same environment.
It happened many times that people reported "I see different errors than those on Travis" - and it turned out that they have different pylint or python version. This is the real benefit of the "huge build". you start to appreciate it when your team of contributors is totally distributed, maintain their own environment and consists of 600 individuals - each with their own setup.
Simply - it looks differently from the point of view of single person and differently from the point of view of team of few hundred people. Sometimes efficiency of team requires some sacrifice of efficiency of single individual.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368532741
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Sure I can try this again - do I select "yes" or "no" at the prompt? I believe I ultimately selected "yes" last time, it failed, and then didn't allow the commit to go through (and it didn't seem to be changed in formatting either).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576459284
Also, could you give feedback on:
> Title Validator — Wrong commit title: adding singularity operator and tests
What is wrong about it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-587935001
Can somebody explain what the task_id (provided as task-id?) is expected to work, be parsed, etc? @mik-laj added it to the test cases last year, and it's not clear if I need to be parsing this for the operator - just trying as it was it failed that the key word argument wasn't valid.
> airflow.exceptions.AirflowException: Invalid arguments were passed to SingularityOperator (task_id: task-id). Invalid arguments were:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576861320
That's OK!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-589395121
looks like the error linked above was one off - a later build didn't trigger it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that it should be a dictionary with all keys and values being strings. It's really easy to add and helps a lot (no need to search for info in docstring) and we can run static types checks that can discover some nasty edge cases. More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring, so it's just a matter of adding them in the constructor like `param: expected_type = default_value` :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368318732
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
For the pre-commit hook, it's hanging on `Forcing response 'no' to 'rebuild CI?'` I don't think I would want to run this locally (hence answering no) but it doesn't seem to get beyond that. Is this expected?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-577337258
[Here are my thoughts](https://gist.github.com/vsoch/599bd51246c2b2d3da350f97c0916efe) @potiuk, I hope that they are helpful! I think you have a challenging problem of wanting to scale an interaction that has many expectations around people, and human interactions that require kindness and patience. But there definitely could be some tweaks (e.g., sticking more to traditional expectations of a review, giving details when it's called for and not expecting pre-request expertise, and either minimizing tooling that is unexpected, defaulting to using the testing environment as the base for giving feedback to the contributor, or removing layers of abstraction so it's less confusing).
Anyway, I outlined the details much better in that document. Please feel free to share!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r367827554
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
+ shutil.rmtree(self.image)
+
+ # If the container failed, raise the exception
+ if result['return_code'] != 0:
+ message = result['message']
+ raise AirflowException('Singularity failed: %s' % message)
+
+ self.log.info('Output from command %s', result['message'])
+
+ def get_command(self):
Review comment:
Pylint will be sad due to no docstring. Please add docstring or make it a protected method (`_get_command`).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585941481
> okay so I'm tracing the kubernetes (runtime) as an example, and I have a quick question. In scripts/ci/in_container/entrypoint_ci.sh I see that given the kubernetes runtime, you are setting some variable for a container name to be the airflow name with suffix "-kubernetes."
>
> and this would suggest the container is brought up inside of this container? Wouldn't it make more sense to have a docker-compose run that runs some equivalent of airflow-testing but with Singularity installed inside?
Yes it would make sense - similar to what we do with other integrations (say kerberos or mongo). What we are doing with kind is a bit different - we are forwarding the host's docker credential to inside the airflow-testing container and we are running the kind command line from inside it. This - in effect - reaches out to the host's docker and set's up the kind cluster using host's docker engine.
With singularity - if it can be run in the host and we can connect to it from inside the airflow-testing container that would be best.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586633655
> Questions:
>
> * where does example_singularity_operator.py go - it used to be under contrib/example_dags but now, that entire folder is empty?
in the meantime we completed AIP-21 and every integration is now moved to 'providers' package.Singularity will need to be moved to providers/singularity
> * this error is triggering for the docs (is it related to the above)? I don't see any script_files with grep, nor do I see how super() relates to the example_singularity_operator.py file.
just look where any of the other providers are added - there is a TOC where you can navigate to your documentation - otherwise users won't be able to find the docs.
>
> ```
> writing additional pages... search/usr/local/lib/python3.6/site-packages/sphinx_rtd_theme/search.html:20: RemovedInSphinx30Warning: To modify script_files in the theme is deprecated. Please insert a <script> tag directly in your theme instead.
this one is misleading it's just a Sphinx internal warning. real warning is much higher in the log (the document is not in any toctree or similar. I think I had a PR that filtered out the false warnings.. I will try to dig I out as I I indeed misleading.
> These docs tests were passing before (but nothing has changed) so what has changed that I don't know about?
That's AIP-21 in the making. For Airflow 2.0 we do a lot of consistency changes. AIP-21 is Al about consistency y between operators, It was planned over last half a year and we just completed I (few more final touches). I was actually leading it band vast majority of the moves happened in last month by members of my team in fact, It's been announced at the devliscie several times, but I guess you aren't following it (and it's no wonder)
> For the mock tests, the author that helped me put "awesome-image" and "awesome-command" in the mock tests - I'm not familiar enough with setting up mocks to understand how this mock is supposed to work (it's not supposed to use real singularity?)
Mocks just pretend to talk to the external integrations and they are all about what unit testing is all about. all unit tests should use mocking. As of recently we also added pytest.mark.integration mark to mark integration tests that should work with real local integrations (such as singularity) we are also in process of adding system tests that are going to talk to real external systems -GCP for example.
All this is explained in detail in TESTING.rst
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586675640
I think we do not have it explained very well - and I am not 100% sure myself. But @mik-laj can help with that.
One more thing @mik-laj - maybe we should add a short explanation/howto on how to add the documentation - where things should be excluded, how to add new packages and guides? For now it's not at all obvious what steps should be taken in case a new provider's package is added. A short "how to add new package documentation" would be great in CONTRIB docs. It does not have to be long - just to explain what to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj edited a comment on issue #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586998761
Here is docs: https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#documentation Please look at the "Known issues" section.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-579315631
Thanks @vsoch ! Really sorry for not getting back before - I've been travelling and missed that last comment.
Fantastic work! I really liked how you explained the steps you did to make it works! Kudos!
> So, although I'm hesitant that something about travis won't provide us with all the permissions that we need, I'm going to give it a try to add a docker compose file to reproduce this. My question for you is how I should go about providing this integration - the examples given previously expose services, and for this case we would want an environment with airflow and singularity in the same space. Do you have thoughts or examples for how to approach this?
Travis and any other environment will provide all that we need. We already run in privileged mode (and pretty much all CI systems allow that in order to make all kinds of docker-in-docker approach). In fact what we already do is we are mounting docker socket to within the airflow-testing so that you can even run docker commands to reach out the host docker engine. This is needed to run kind (Kubernetes-In-Docker).
> And then given that we can add a custom container, it would need to use one of the Singularity bases (note they are alpine), and then also include dependencies for airflow (or is the install already handled given the container, and happens before the testing?) Either way, if customization is needed on top of the Singularity base (that isn't handled by the testing suite) what would be the best way to do this - building the image elsewhere and using here, or building on demand?
Hmm, I'd assume that having a base Singularity should be enough and that we can reach it via "singularity" host name via docker compose. But I understand you need to build in some Airflow dependencies in? Am I right? What kind of dependencies you need to build in that container? Is this like local airflow sources or some airflow dependencies? Excuse my ignorance as I do not know much about singularity.
We have few approaches we can get:
1) In case of Kubernetes (Kind) we are using RUNTIME="kubernetes" env and we are checking that in the initial CI scripts - if the RUNTIME="kubernetes" we will install kubernetes-in-docker cluster and provision it with the latest Airflow locally built CI image and then we run our tests with this. This is most complex thing we do and I hope we do not have to do the same.
2) If those are about some small packages to install at running singularity after it is started already, we could simply do it in the CI scripts when we start up and see "INTEGRATION_SINGULARITY" is set to "true" (this will be properly set to true if you add it similarly to other integrations of ours and when --integration singularity switch is used).
3) If you need something pre-backed in the Docker image itself then I think we need a separate Dockerfile with FROM: singularity and some steps to build the dockerfile before docker-compose is run (and refer to the newly built image in Docker compose. My best bet if that is the case, that would be that you build and test such images locally for local testing and once it works I can help to integrate it in the CI scripts properly (You can try yourself - this will likely be just appropriate 'docker build' command executed only when --integration singularity is used)
I hope this is helpful!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576851524
Writing up this robust feedback and researching the CI solutions is going to take me some time, just to keep you updated I won't be able to work on this promptly, I need to prioritize work for my job. But I'll be back, stay tuned! :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-577373099
okay, I've taken a look at the docker-compose approach. Specifically, I've created a `integration-singularity.yml` file that uses a container base that provides Singularity (v3.5.1).
```yaml
version: "2.2"
services:
singularity:
image: quay.io/singularity/singularity:v3.5.1
command: -F /etc/alpine-release
entrypoint: /usr/bin/tail
privileged: true
volumes:
- /dev/urandom:/dev/random # Required to get non-blocking entropy source
```
Note that I'm using privileged, which likely requires the sudo:true parameter for the testing setup. The traditional entrypoint is the Singularity executable, so I have a basic entrypoint/command to just keep the container running (and allow me to shell inside). Even outside of testing (which is fairly complicated and I'm hoping to not need to reproduce it locally) we can test if the basic interaction with Singularity would work inside the container.
```bash
docker-compose --log-level INFO -f "scripts/ci/docker-compose/integration-singularity.yml" up -d
```
Check that everything went up ok...
```bash
docker-compose --log-level INFO -f "scripts/ci/docker-compose/integration-singularity.yml" logs
Attaching to docker-compose_singularity_1_7d898ecbdbb0
singularity_1_7d898ecbdbb0 | 3.10.3
```
And that we continued running...
```bash
$ docker-compose --log-level INFO -f "scripts/ci/docker-compose/integration-singularity.yml" ps
Name Command State Ports
--------------------------------------------------------------------------------------
docker-compose_singularity_1_7d898ecbdbb0 /usr/bin/tail -F /etc/alpi ... Up
```
Great! And then we can shell inside. We confirm the version of singularity.
```bash
$ docker exec -it 0081d687eeb1 bash
bash-5.0# which singularity
/usr/local/bin/singularity
bash-5.0# singularity --version
singularity version 3.5.1
```
The basic thing we would need to do is start instances. We can actually build the SIF binary successfully, and let's do that first, pulling the container:
```bash
$ docker exec -it 3e73d2d5b41e bash
bash-5.0# which singularity
/usr/local/bin/singularity
bash-5.0# singularity pull docker://busybox
INFO: Converting OCI blobs to SIF format
INFO: Starting build...
Getting image source signatures
Copying blob bdbbaa22dec6 done
Copying config b075e856be done
Writing manifest to image destination
Storing signatures
2020/01/22 20:24:24 info unpack layer: sha256:bdbbaa22dec6b7fe23106d2c1b1f43d9598cd8fc33706cc27c1d938ecd5bffc7
INFO: Creating SIF file...
INFO: Build complete: busybox_latest.sif
bash-5.0#
```
Now let's start the instance.
```bash
bash-5.0# singularity instance start busybox_latest.sif busybox
ERROR: container cleanup failed: no instance found with name busybox
FATAL: container creation failed: mount /etc/localtime->/etc/localtime error: while mounting /etc/localtime: mount source /etc/localtime doesn't exist
FATAL: failed to start instance: while running /usr/local/libexec/singularity/bin/starter: exit status 255
```
okay, let's trace back and add this to our docker-compose yml file, we will mount as a volume from the host and redo the above.
```yaml
version: "2.2"
services:
singularity:
image: quay.io/singularity/singularity:v3.5.1
command: -F /etc/alpine-release
entrypoint: /usr/bin/tail
privileged: true
volumes:
- /etc/localtime:/etc/localtime
```
Zooming ahead, let's recreate and then start the instance:
```bash
bash-5.0# singularity instance start busybox_latest.sif busybox
INFO: instance started successfully
```
Success!
```bash
bash-5.0# singularity instance list
INSTANCE NAME PID IP IMAGE
busybox 78 /go/busybox_latest.sif
```
And we can exec to it:
```bash
bash-5.0# singularity exec instance://busybox echo "HELLO"
HELLO
```
So, although I'm hesitant that something about travis won't provide us with all the permissions that we need, I'm going to give it a try to add a docker compose file to reproduce this. My question for you is how I should go about providing this integration - the examples given previously expose services, and for this case we would want an environment with airflow and singularity in the same space. Do you have thoughts or examples for how to approach this?
And then given that we can add a custom container, it would need to use one of the Singularity bases (note they are alpine), and then also include dependencies for airflow (or is the install already handled given the container, and happens before the testing?) Either way, if customization is needed on top of the Singularity base (that isn't handled by the testing suite) what would be the best way to do this - building the image elsewhere and using here, or building on demand?
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] boring-cyborg[bot] commented on issue #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590050707
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7191: [AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368289791
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
Type hints allow us to easily indicate what types arguments have and if they are necessary:
https://github.com/apache/airflow/blob/e2524fac59df1a405a38f7f71b99436b44a47378/airflow/providers/google/cloud/operators/pubsub.py#L113-L128
In above example you can see that `labels` argument is `Optional` and if passed should be the type of `Dict[str, str]` meaning that all keys and values should be strings. It's really easy to add and helps a lot (no need to search for info in docstring). More information here: https://docs.python.org/3/library/typing.html
You have already figured types in `:type param_name:` in the docstring :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #7191: [AIRFLOW-4030]
second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576523641
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=h1) Report
> Merging [#7191](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/44769dc172ff0438240dc64b5e5f6cb7c62f8af1?src=pr&el=desc) will **decrease** coverage by `52.68%`.
> The diff coverage is `17.97%`.
[](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7191 +/- ##
===========================================
- Coverage 85.37% 32.69% -52.69%
===========================================
Files 723 759 +36
Lines 39546 40008 +462
===========================================
- Hits 33764 13080 -20684
- Misses 5782 26928 +21146
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ntrib/example\_dags/example\_singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3Npbmd1bGFyaXR5X29wZXJhdG9yLnB5) | `0% <0%> (ø)` | |
| [airflow/contrib/operators/singularity\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9zaW5ndWxhcml0eV9vcGVyYXRvci5weQ==) | `21.05% <21.05%> (ø)` | |
| [...low/contrib/operators/wasb\_delete\_blob\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy93YXNiX2RlbGV0ZV9ibG9iX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [...flow/contrib/example\_dags/example\_qubole\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3F1Ym9sZV9zZW5zb3IucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...ample\_dags/example\_emr\_job\_flow\_automatic\_steps.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX2Vtcl9qb2JfZmxvd19hdXRvbWF0aWNfc3RlcHMucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [...contrib/example\_dags/example\_papermill\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2V4YW1wbGVfZGFncy9leGFtcGxlX3BhcGVybWlsbF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/hooks/webhdfs\_hook.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9ob29rcy93ZWJoZGZzX2hvb2sucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/sensors/emr\_base\_sensor.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL3NlbnNvcnMvZW1yX2Jhc2Vfc2Vuc29yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/contrib/operators/gcs\_list\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9nY3NfbGlzdF9vcGVyYXRvci5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [airflow/operators/pig\_operator.py](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvcGlnX29wZXJhdG9yLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [629 more](https://codecov.io/gh/apache/airflow/pull/7191/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=footer). Last update [44769dc...5ab4861](https://codecov.io/gh/apache/airflow/pull/7191?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-586169342
> High level feedback since this was a topic earlier:
>
> * the extra linting for the Dockerfile, which comes down to fairly trivial things like using cd in a command for one layer and using curl instead of wget, has led to several additional errors that are frustrating to deal with. If I knew the project perfectly sure I might have known these in advance, but for a new contributor that gets these bugs (which are somewhat subjective) that then has to figure them out and fix, it's an extra pain that simply doesn't need to exist.
> * given the long duration of a PR, the addition of new linters / tests makes it even more confusing.
>
> I'm happy to keep providing this feedback, and if you don't want it let me know!
Feedback is welcome :).
I understand it's a bit of pain with linting, but I would treat it as a great learning experience. For me this is the best trainer I have - when I make a mistake or don't do something very well and can do better, I get feedback. That's the case with linting - I've learned a lot of good practices from linting - hadolint for Docker and especially shellcheck for bash.
And especially that those lint rules have very good explanation rationale and most of them has the effect that they prevent errors that others might have problems with. For example "using cd" has a good explanation and I think it's a fantastic practice to use WORKDIR instead of cd in Dockerfiles as it makes it more controllable - you always know which directory you are in without having to parse the shell statement. Here is more detailed explanation with example and rationale: https://github.com/hadolint/hadolint/wiki/DL3003
You might of course disagree with particular rules (and you have full right to do so) but we also have the rule in community "disagree but engage" - community decisions are more important than individual preferences. That's one of the things that I had to learn when I joined the community - there are a number of things I do not agree with but then after community decided otherwise i follow and even engage with). On the other hand there were a few rules that were annoying and we raised a proposal to remove them eventually, so there is still place for discussing them in the devlist - and if you have good reasoning why the rules should be removed, I am sure people who will be interested in it will have opinions and maybe decision will be made to disable those rules.
* Regarding adding new checks - here again community is more important. When you look at it from your individual point of view, it's bad, but if you look at it from the community, there are more than 100 PRs opened at any time so we would never had a chance to improve - steady improvements are crucial, if you don't improve you actually deteriorate, so while annoying, it's also expected that after rebase you might have to adapt to some new rules. That's actually a sign of healthy project. Also if you had pre-commits installed and working, those "encoding" lines would be removed automatically for you. Whenever we add new rules we try to make them self-fixing and having pre-commit is one of the reasons for that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-590050910
Indeed Awesome work 🎉 🎉 🎉 🎉 🎉 🎉 . Likewise :). Thanks for cooperation on that one !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368318895
##########
File path: tests/contrib/operators/test_singularity_operator.py
##########
@@ -0,0 +1,180 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+import six
+
+from parameterized import parameterized
+from spython.instance import Instance
+from airflow import AirflowException
+
+try:
+ from airflow.contrib.operators.singularity_operator import SingularityOperator
+except ImportError:
+ pass
Review comment:
Let me know how you'd suggest that I proceed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368312552
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
okay, added! I really appreciate the quick examples, they made it much easier to digest the new syntax fairly quickly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576459203
The linting appears to be all set, but since Singularity is installed, the tests are obviously going to fail. Let me know how you would like to proceed - it's a non trivial thing to get Singularity installed from within Travis.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-576471260
You definitely can’t get a fully working Singularity from within Docker (even with privileged it’s a bit janky) so I’ll need to look at the other options and decide what is most logical to try first. I’ll take a look tomorrow, thanks for the clearly outlined options!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on issue #7191: [AIRFLOW-4030] second
attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on issue #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#issuecomment-585869248
ping...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368312177
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
Review comment:
okay I'll give this a shot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] vsoch commented on a change in pull request #7191:
[AIRFLOW-4030] second attempt to add singularity to airflow
Posted by GitBox <gi...@apache.org>.
vsoch commented on a change in pull request #7191: [AIRFLOW-4030] second attempt to add singularity to airflow
URL: https://github.com/apache/airflow/pull/7191#discussion_r368025298
##########
File path: airflow/contrib/operators/singularity_operator.py
##########
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.exceptions import AirflowException
+from airflow.models import BaseOperator
+from airflow.utils.decorators import apply_defaults
+from spython.main import Client
+import shutil
+import ast
+import os
+
+
+class SingularityOperator(BaseOperator):
+ """
+ Execute a command inside a Singularity container
+
+ Singularity has more seamless connection to the host than Docker, so
+ no special binds are needed to ensure binding content in the user $HOME
+ and temporary directories. If the user needs custom binds, this can
+ be done with --volumes
+
+ :param image: Singularity image or URI from which to create the container.
+ :type image: str
+ :param auto_remove: Delete the container when the process exits
+ The default is False.
+ :type auto_remove: bool
+ :param command: Command to be run in the container. (templated)
+ :type command: str or list
+ :param start_command: start command to pass to the container instance
+ :type start_command: string or list
+ :param environment: Environment variables to set in the container. (templated)
+ :type environment: dict
+ :param force_pull: Pull the image on every run. Default is False.
+ :type force_pull: bool
+ :param volumes: List of volumes to mount into the container, e.g.
+ ``['/host/path:/container/path', '/host/path2:/container/path2']``.
+ :param options: other flags (list) to provide to the instance start
+ :type options: list
+ :param working_dir: Working directory to
+ set on the container (equivalent to the -w switch the docker client)
+ :type working_dir: str
+ """
+ template_fields = ('command', 'environment',)
+ template_ext = ('.sh', '.bash',)
+
+ @apply_defaults
+ def __init__(
+ self,
+ image,
+ api_version=None,
+ command=None,
+ start_command=None,
+ environment=None,
+ pull_folder=None,
+ force_pull=False,
+ volumes=None,
+ options=None,
+ working_dir=None,
+ auto_remove=False,
+ *args,
+ **kwargs):
+
+ super(SingularityOperator, self).__init__(*args, **kwargs)
+ self.api_version = api_version
+ self.auto_remove = auto_remove
+ self.command = command
+ self.start_command = start_command
+ self.environment = environment or {}
+ self.force_pull = force_pull
+ self.image = image
+ self.instance = None
+ self.options = options or []
+ self.pull_folder = pull_folder
+ self.volumes = volumes or []
+ self.working_dir = working_dir
+ self.cli = None
+ self.container = None
+
+ def execute(self, context):
+ self.log.info('Preparing Singularity container %s', self.image)
+ self.cli = Client
+
+ if not self.command:
+ raise AirflowException('You must define a command.')
+
+ # Pull the container if asked, and ensure not a binary file
+ if self.force_pull and not os.path.exists(self.image):
+ self.log.info('Pulling container %s', self.image)
+ image, lines = self.cli.pull(self.image, stream=True)
+ for line in lines:
+ self.log.info(line)
+
+ # Move the container to where it's desired
+ if self.pull_folder is not None:
+ self.image = os.path.join(self.pull_folder, os.path.basename(image))
+ shutil.move(image, self.image)
+
+ # Prepare list of binds
+ for bind in self.volumes:
+ self.options = self.options + ['--bind', bind]
+
+ # Does the user want a custom working directory?
+ if self.working_dir is not None:
+ self.options = self.options + ['--workdir', self.working_dir]
+
+ # Export environment before instance is run
+ for enkey, envar in self.environment.items():
+ self.log.debug('Exporting %s=%s', envar, enkey)
+ os.putenv(enkey, envar)
+ os.environ[enkey] = envar
+
+ # Create a container instance
+ self.log.debug('Options include: %s', self.options)
+ self.instance = self.cli.instance(self.image,
+ options=self.options,
+ args=self.start_command,
+ start=False)
+
+ self.instance.start()
+ self.log.info(self.instance.cmd)
+ self.log.info('Created instance %s from %s', self.instance, self.image)
+
+ self.log.info('Running command %s', self.get_command())
+ self.cli.quiet = True
+ result = self.cli.execute(self.instance,
+ self.get_command(),
+ return_result=True)
+
+ # Stop the instance
+ self.log.info('Stopping instance %s', self.instance)
+ self.instance.stop()
+
+ if self.auto_remove is True:
+ if os.path.exists(self.image):
+ shutil.rmtree(self.image)
+
+ # If the container failed, raise the exception
+ if result['return_code'] != 0:
+ message = result['message']
+ raise AirflowException('Singularity failed: %s' % message)
+
+ self.log.info('Output from command %s', result['message'])
+
+ def get_command(self):
Review comment:
I'll make it a protected method - I don't see it being useful for the user to call, but something that is more private for the operator.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services