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 2022/11/20 19:10:28 UTC
[GitHub] [airflow] aaronabraham311 opened a new pull request, #27805: Automatically save and allow restore of recent DAG run configs
aaronabraham311 opened a new pull request, #27805:
URL: https://github.com/apache/airflow/pull/27805
closes: #27714
Note: we found that the restored recent config isn't automatically formatted because CodeMirror doesn't support that anymore. We could add js-beautify but we were unsure if we should add this library for this singular fix. It would also only work for ES10. If we want more backwards compatability, let us know and we can re-write.
cc: @jh242
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035267390
##########
airflow/www/utils.py:
##########
@@ -133,17 +133,20 @@ def get_mapped_summary(parent_instance, task_instances):
}
+def get_dag_run_conf(conf: Any):
Review Comment:
done!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aaronabraham311 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
aaronabraham311 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1326792202
@pierrejeambrun Oh that's great! Is there any example on how to access the DagRun configs from the db? Or is there a code snippet that we can use as an example?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1037281152
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,32 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ let { value } = e.target;
+ try {
+ const json = JSON.parse(value);
+ value = JSON.stringify(json, null, 2);
+ } catch (err) {
+ // any error is non-json parseable
+ value = 'Error: config could not be parsed';
Review Comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aaronabraham311 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
aaronabraham311 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1331119159
Hi @pierrejeambrun @bbovenzi! We have addressed your requested changes. Please let us know if we can begin merging this! We would like to merge this in by end of this week if possible as we won't be able to push this through afterwards.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1332680436
@jh242 Can you also fix the failing text. The extracted function `get_dag_run_conf` is not exactly doing what was done before directly in the `encode_dag_run`. I recommend keeping the same structure.
(There is a difference if the dagrun is an empty object)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035309251
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,24 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ document.querySelector('.CodeMirror').CodeMirror.setValue(e.target.value);
Review Comment:
Hey yeah! I wanted to do this as well, but code formatting is a function that CodeMirror no longer supports. The other alternative is adding another JS library just to do this formatting, but I don't think we need this overhead (let me know if we already have one that can do it!).
The other alternative is writing a bespoke function to do JSON formatting but I didn't think it was worth the trouble.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1334353906
updated our branch to be up-to-date with airflow. Thanks again @bbovenzi @pierrejeambrun :D
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1326606738
I disagree with the db/API endpoint idea. I think it's too much overhead for a minor feature that seems intended to save some time for DAGs with small configs that the user didn't expect to run multiple times. I feel like the ability to copy/paste configurations from [here](https://github.com/apache/airflow/pull/27639) also overlaps this feature. My suggestion is that we can either save recent configs in session storage by DAG, or in local storage but limited to a certain number of total recent configs across all DAGs.
Additionally, Aaron and I are on a bit of a short timeline and likely won't have time to implement a backend-supported version of this feature, but if that's the direction we really want to go, we can get started on it and see where it goes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1037236111
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,32 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ let { value } = e.target;
+ try {
+ const json = JSON.parse(value);
+ value = JSON.stringify(json, null, 2);
+ } catch (err) {
+ // any error is non-json parseable
+ value = 'Error: config could not be parsed';
Review Comment:
We can throw a `console.error()` here, but lets leave the value as a single string if we can't parse 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1036183188
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
I think `.distinct(DagRun.conf)` 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1324115660
Is it relevent to see 'previous config' of all other dag ? Should this be on a per dag basis ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1326777609
Now that you mention it, DagRun conf are already stored and available for each run. Isn't it easier to just retrieve them and provide them to the `trigger.html` template directly ? We also have a lot of control of what conf should be retrieved (5 most recent conf for a specific dag, 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] aaronabraham311 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
aaronabraham311 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1324285337
> Also wondering if the localstorage is the correct place for this. Storing all configs of each manual run for each dag could be pretty heavy. (localstorage is limited to 5M). Also loading all of this on the page could be quite long (storage is synchronous if i am not mistaken so ready all of that from it could be expensive on a UI point of view)
@pierrejeambrun Do you have an alternative suggestion of storing recent configs asides from localstorage?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1324128161
Also wondering if the localstorage is the correct place for this. Storing all configs of each manual run for each dag could be pretty heavy. (localstorage is limited to 5M). Also loading all of this on the page could be quite long (storage is synchronous if i am not mistaken so ready all of that from it could be expensive on a UI point of view)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035331621
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
I'm a bit of an SQLalchemy noob--I thought that dropping in a .distinct after (or before) the .order_by clause would produce what we needed, but it doesn't seem to be working and I'm not sure why. Could I get a bit of guidance on 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1036383024
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
I've tried this and it did not. Removing the .order_by made it work though.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1321775152
If you don't mind, you can add before and after pics
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1033021999
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,24 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ document.querySelector('.CodeMirror').CodeMirror.setValue(e.target.value.replaceAll("'", '"'));
Review Comment:
To avoid manually replacing the quotes, you can `json.dumps(conf)`. See what is done in the grid_data endpoint, and my comments bellow.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1328221584
Hello @jh242,
Thank you for the update. Here you are loading the grid_data info (which has a lot of extra stuff on task instances etc.), just to retrieve the 'configs'. As you said you could end up with less than 10 confs (because of duplicate), or even with no conf at all if the last 10 run were scheduled.
Here we statically render a template, there is usually no need to go through an API call. What you could do is, inside the view that renders the `trigger.html`, make your custom request to the db to retrieve the conf you like. (Filter on confs that are not of type json, deduplicate them with a distrinc(), fetch only the last runs that were 'manually triggered', with a conf `not None`) etc. When you have your configurations, you can pass this as an extra variable to the `trigger.html` template. Then update the template so it can use this variable to render extra options in the drop down.
You can check the `def index` (/home) view and check how we directly pass variable to the template for rendering. (dags, tags etc).
Maybe I missed something, but this should make the code even cleaner and simple :)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1332840168
@pierrejeambrun @bbovenzi My latest commit should address all of the above issues! Thanks so much again guys. Would really appreciate getting this merged before Friday if possible--we're contributing as part of a university course and it would be great to have something to show for our efforts :D
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1322325623
Note that when a recent configuration is selected, it is not automatically formatted as CodeMirror no longer supports this functionality. Another library can be introduced to remedy this (something like js-beautify) but seems unnecessary.
I don't have any before images, but the only difference is that the "Recent Configurations" input is not there.
Please see these after images:
<img width="1193" alt="Screenshot 2022-11-21 at 11 09 55 AM" src="https://user-images.githubusercontent.com/34525842/203105919-39ac8022-604f-4c76-873a-ca251864eb81.png">
<img width="1196" alt="Screenshot 2022-11-21 at 11 10 14 AM" src="https://user-images.githubusercontent.com/34525842/203105988-81630bd9-9df4-4099-9ac8-845410458532.png">
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035305777
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,24 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ document.querySelector('.CodeMirror').CodeMirror.setValue(e.target.value);
Review Comment:
I wonder if we should try to format the config JSON so it doesn't appear as a single line
```
function setRecentConfig(e) {
let { value } = e.target;
try {
const json = JSON.parse(value);
value = JSON.stringify(json, null, 2);
} catch (err) {
console.err(err);
}
document.querySelector('.CodeMirror').CodeMirror.setValue(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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035314071
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,24 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ document.querySelector('.CodeMirror').CodeMirror.setValue(e.target.value);
Review Comment:
The function above should do it. We just need that try/catch block to capture thrown errors when it is not valid json.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1334504199
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035331621
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
I'm a bit of an SQLalchemy noob--I thought that dropping in a .distinct after (or before) the .order_by clause would produce what we needed, but it doesn't seem to be working and I'm not sure why. Could I get a bit of guidance on this @bbovenzi ? Thanks in advance!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1328323235
Thanks @pierrejeambrun , that's an excellent point. My latest commit implements your 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1033021606
##########
airflow/www/templates/airflow/trigger.html:
##########
@@ -44,12 +44,23 @@ <h2>Trigger DAG: {{ dag_id }}</h2>
<div class="form-group row">
<div class="col-md-2">
<label for="run_id">Run id (Optional)</label>
- <input type="text" class="form-control" placeholder="Run ID" name="run_id" >
+ <input type="text" class="form-control" placeholder="Run ID" name="run_id">
+ </div>
+ </div>
+ <div class="form-group row">
+ <div class="col-md-2">
+ <label for="recent_configs">Recent Configurations</label>
+ <select class="form-control" name="recent_configs" id="recent_configs">
+ <option value="">None</option>
+ {% for conf in recent_confs %}
+ <option value="{{conf}}">{{ conf }}</option>
+ {% endfor %}
+ </select>
</div>
</div>
<label for="conf">Configuration JSON (Optional, must be a dict object)</label>
<textarea class="form-control" name="conf" id="json">{{ conf }}</textarea>
- </div>
+
Review Comment:
This div was here to adding some spacing after the json editor. (It wasn't working properly indeed). Maybe adding a line break (<br>), currently it's quite compact :):
##########
airflow/www/templates/airflow/trigger.html:
##########
@@ -44,12 +44,23 @@ <h2>Trigger DAG: {{ dag_id }}</h2>
<div class="form-group row">
<div class="col-md-2">
<label for="run_id">Run id (Optional)</label>
- <input type="text" class="form-control" placeholder="Run ID" name="run_id" >
+ <input type="text" class="form-control" placeholder="Run ID" name="run_id">
+ </div>
+ </div>
+ <div class="form-group row">
+ <div class="col-md-2">
+ <label for="recent_configs">Recent Configurations</label>
+ <select class="form-control" name="recent_configs" id="recent_configs">
+ <option value="">None</option>
Review Comment:
Maybe we can display something like 'empty config' instead of None that doesn't provide much information to the user ?
##########
airflow/www/views.py:
##########
@@ -1855,6 +1855,19 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf is not None,
Review Comment:
I don't think empty objects are relevent here. (also update the `is not None` syntax)
```suggestion
DagRun.conf.isnot(None),
DagRun.conf != {}
```
##########
airflow/www/static/js/trigger.js:
##########
@@ -20,13 +20,24 @@
/* global document, CodeMirror, window */
const textArea = document.getElementById('json');
+const recentConfigList = document.getElementById('recent_configs');
const minHeight = 300;
const maxHeight = window.innerHeight - 450;
const height = maxHeight > minHeight ? maxHeight : minHeight;
CodeMirror.fromTextArea(textArea, {
lineNumbers: true,
- mode: { name: 'javascript', json: true },
+ mode: {
+ name: 'javascript',
+ json: true,
+ },
gutters: ['CodeMirror-lint-markers'],
lint: true,
-}).setSize(null, height);
+})
+ .setSize(null, height);
+
+function setRecentConfig(e) {
+ document.querySelector('.CodeMirror').CodeMirror.setValue(e.target.value.replaceAll("'", '"'));
Review Comment:
To avoid manually replacing the quotes, you can `json.dumps(conf)`. See what is done in the grid_data endpoint, and my other comments.
##########
airflow/www/views.py:
##########
@@ -1855,6 +1855,19 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf is not None,
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
+ )
+ recent_confs = [getattr(conf, "conf") for conf in recent_confs]
Review Comment:
Conf are not always json compatible. I believe they can be plain 'str'. check `def encode_dag_run` func. We may want to also handle this case. Maybe extract a reusable function that will handle the conf. (`json.dumps` if possible, otherwise str, and return `conf_is_json`, and skip the conf that are not json in the response)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1033022940
##########
airflow/www/views.py:
##########
@@ -1855,6 +1855,19 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf is not None,
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
+ )
+ recent_confs = [getattr(conf, "conf") for conf in recent_confs]
Review Comment:
Conf are not always json compatible. I believe they can be plain `str`. check `def encode_dag_run` func. We may want to also handle this case. Maybe we can extract a reusable function that will handle the conf the same way that is done in `encode_dag_run`. (`json.dumps` if possible, otherwise `str`, and also returns `conf_is_json`, then we skip the conf that are not json in the response)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1322924521
What happens if the previous config is big? I wonder if we should simply show a datetime or sorts for each saved entry instead.
Also do we need a way for the user to clear saved configs?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1036383024
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
~~I've tried this and it did not. Removing the .order_by made it work though.~~ Works 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #27805:
URL: https://github.com/apache/airflow/pull/27805
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1033021606
##########
airflow/www/templates/airflow/trigger.html:
##########
@@ -44,12 +44,23 @@ <h2>Trigger DAG: {{ dag_id }}</h2>
<div class="form-group row">
<div class="col-md-2">
<label for="run_id">Run id (Optional)</label>
- <input type="text" class="form-control" placeholder="Run ID" name="run_id" >
+ <input type="text" class="form-control" placeholder="Run ID" name="run_id">
+ </div>
+ </div>
+ <div class="form-group row">
+ <div class="col-md-2">
+ <label for="recent_configs">Recent Configurations</label>
+ <select class="form-control" name="recent_configs" id="recent_configs">
+ <option value="">None</option>
+ {% for conf in recent_confs %}
+ <option value="{{conf}}">{{ conf }}</option>
+ {% endfor %}
+ </select>
</div>
</div>
<label for="conf">Configuration JSON (Optional, must be a dict object)</label>
<textarea class="form-control" name="conf" id="json">{{ conf }}</textarea>
- </div>
+
Review Comment:
This div was here to add some spacing after the json editor. (It wasn't working properly indeed). Maybe adding a line break (<br>), currently it's quite compact :):
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035228975
##########
airflow/www/utils.py:
##########
@@ -133,17 +133,20 @@ def get_mapped_summary(parent_instance, task_instances):
}
+def get_dag_run_conf(conf: Any):
Review Comment:
return type
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1321217091
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1328132103
> @aaronabraham311 There are a lot of examples querying resource from the db in the [`views.py`](https://github.com/apache/airflow/blob/main/airflow/www/views.py) file. In this case DagRun should have what you need. In the example you mentioned above (json config displayed in the dagrun details), its coming from the [`grid_data`](https://github.com/apache/airflow/blob/main/airflow/www/views.py#L3487) view of the same file :)
I've implemented this approach and limited the number of requested recent DAG runs to 10... Because there may be duplicate recent configs this means we won't necessarily get 10 different recent configs. Take a look!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035310127
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
Whoops, yep! Somehow that slipped my mind.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jh242 commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
jh242 commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1328395650
Hey @pierrejeambrun , thanks so much for your guidance so far. I've implemented your suggestions, including the reusable function for checking if dagRun configs are JSON or not. I'm still a little bit confused on serializing the response from my query as I haven't worked with Flask before and I wasn't sure how to test a string dagRun config, so that may need more attention.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #27805:
URL: https://github.com/apache/airflow/pull/27805#discussion_r1035307136
##########
airflow/www/views.py:
##########
@@ -1895,6 +1895,20 @@ def trigger(self, session=None):
flash(f"Cannot create dagruns because the dag {dag_id} has import errors", "error")
return redirect(origin)
+ recent_confs = (
+ session.query(DagRun.conf)
+ .filter(
+ DagRun.dag_id == dag_id,
+ DagRun.run_type == DagRunType.MANUAL,
+ DagRun.conf.isnot(None),
+ )
+ .order_by(DagRun.execution_date.desc())
+ .limit(5)
+ .all()
Review Comment:
Can we change this to the first 5 unique configs?
If we can now select old configs, a lot of the options would just be the same.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1324672374
@aaronabraham311, one other obvious way would be to persist them in the db, and expose them through the rest api.
This requires more effort to implement, and I am not sure all db support json type. This is just an idea, wondering what others think.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #27805: Automatically save and allow restore of recent DAG run configs
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #27805:
URL: https://github.com/apache/airflow/pull/27805#issuecomment-1326861132
@aaronabraham311 There is a lot of example querying resource from the db in the views.py file. In this case DagRun should have what you need. In the example you mentioned above (json config displayed in the dagrun details), its coming from the grid_data view of that 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org